From 754f04cac362417b157e30d5dc4046b5ec92060c Mon Sep 17 00:00:00 2001 From: Cristian Marussi Date: Thu, 16 Jun 2022 18:03:47 +0100 Subject: [PATCH 1/3] firmware: arm_scmi: Relax CLOCK_DESCRIBE_RATES out-of-spec checks A reply to CLOCK_DESCRIBE_RATES issued against a non rate-discrete clock should be composed of a triplet of rates descriptors (min/max/step) returned all in one reply message. This is not always the case when dealing with some SCMI server deployed in the wild: relax such constraint while maintaining memory safety by checking carefully the returned payload size. While at that cleanup a stale debug printout. Link: https://lore.kernel.org/r/20220616170347.2800771-1-cristian.marussi@arm.com Fixes: 7bc7caafe6b1 ("firmware: arm_scmi: Use common iterators in the clock protocol") Tested-by: Robin Murphy Signed-off-by: Cristian Marussi Signed-off-by: Sudeep Holla --- drivers/firmware/arm_scmi/clock.c | 26 +++++++++++++++++++++++++- drivers/firmware/arm_scmi/driver.c | 1 + drivers/firmware/arm_scmi/protocols.h | 3 +++ 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c index c7a83f6e38e5..3ed7ae0d6781 100644 --- a/drivers/firmware/arm_scmi/clock.c +++ b/drivers/firmware/arm_scmi/clock.c @@ -194,6 +194,7 @@ static int rate_cmp_func(const void *_r1, const void *_r2) } struct scmi_clk_ipriv { + struct device *dev; u32 clk_id; struct scmi_clock_info *clk; }; @@ -223,6 +224,29 @@ iter_clk_describe_update_state(struct scmi_iterator_state *st, st->num_returned = NUM_RETURNED(flags); p->clk->rate_discrete = RATE_DISCRETE(flags); + /* Warn about out of spec replies ... */ + if (!p->clk->rate_discrete && + (st->num_returned != 3 || st->num_remaining != 0)) { + dev_warn(p->dev, + "Out-of-spec CLOCK_DESCRIBE_RATES reply for %s - returned:%d remaining:%d rx_len:%zd\n", + p->clk->name, st->num_returned, st->num_remaining, + st->rx_len); + + /* + * A known quirk: a triplet is returned but num_returned != 3 + * Check for a safe payload size and fix. + */ + if (st->num_returned != 3 && st->num_remaining == 0 && + st->rx_len == sizeof(*r) + sizeof(__le32) * 2 * 3) { + st->num_returned = 3; + st->num_remaining = 0; + } else { + dev_err(p->dev, + "Cannot fix out-of-spec reply !\n"); + return -EPROTO; + } + } + return 0; } @@ -255,7 +279,6 @@ iter_clk_describe_process_response(const struct scmi_protocol_handle *ph, *rate = RATE_TO_U64(r->rate[st->loop_idx]); p->clk->list.num_rates++; - //XXX dev_dbg(ph->dev, "Rate %llu Hz\n", *rate); } return ret; @@ -275,6 +298,7 @@ scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph, u32 clk_id, struct scmi_clk_ipriv cpriv = { .clk_id = clk_id, .clk = clk, + .dev = ph->dev, }; iter = ph->hops->iter_response_init(ph, &ops, SCMI_MAX_NUM_RATES, diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c index c1922bd650ae..8b7ac6663d57 100644 --- a/drivers/firmware/arm_scmi/driver.c +++ b/drivers/firmware/arm_scmi/driver.c @@ -1223,6 +1223,7 @@ static int scmi_iterator_run(void *iter) if (ret) break; + st->rx_len = i->t->rx.len; ret = iops->update_state(st, i->resp, i->priv); if (ret) break; diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h index c679f3fb8718..51c31379f9b3 100644 --- a/drivers/firmware/arm_scmi/protocols.h +++ b/drivers/firmware/arm_scmi/protocols.h @@ -179,6 +179,8 @@ struct scmi_protocol_handle { * @max_resources: Maximum acceptable number of items, configured by the caller * depending on the underlying resources that it is querying. * @loop_idx: The iterator loop index in the current multi-part reply. + * @rx_len: Size in bytes of the currenly processed message; it can be used by + * the user of the iterator to verify a reply size. * @priv: Optional pointer to some additional state-related private data setup * by the caller during the iterations. */ @@ -188,6 +190,7 @@ struct scmi_iterator_state { unsigned int num_remaining; unsigned int max_resources; unsigned int loop_idx; + size_t rx_len; void *priv; }; From 75c8f430d8596cf16634337fec8aec50e7b0feed Mon Sep 17 00:00:00 2001 From: Vincent Guittot Date: Fri, 24 Jun 2022 09:45:49 +0200 Subject: [PATCH 2/3] firmware: arm_scmi: Fix response size warning for OPTEE transport Some protocols check the response size with the expected value but optee shared memory doesn't return such size whereas it is available in the optee output buffer. As an example, the base protocol compares the response size with the expected result when requesting the list of protocol which triggers a warning with optee shared memory: arm-scmi firmware:scmi0: Malformed reply - real_sz:116 calc_sz:4 (loop_num_ret:4) Save the output buffer length and use it when fetching the answer. Link: https://lore.kernel.org/r/20220624074549.3298-1-vincent.guittot@linaro.org Reviewed-by: Etienne Carriere Reviewed-by: Cristian Marussi Signed-off-by: Vincent Guittot Signed-off-by: Sudeep Holla --- drivers/firmware/arm_scmi/optee.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/firmware/arm_scmi/optee.c b/drivers/firmware/arm_scmi/optee.c index b503c22cfd32..8abace56b958 100644 --- a/drivers/firmware/arm_scmi/optee.c +++ b/drivers/firmware/arm_scmi/optee.c @@ -117,6 +117,7 @@ struct scmi_optee_channel { u32 channel_id; u32 tee_session; u32 caps; + u32 rx_len; struct mutex mu; struct scmi_chan_info *cinfo; union { @@ -302,6 +303,9 @@ static int invoke_process_msg_channel(struct scmi_optee_channel *channel, size_t return -EIO; } + /* Save response size */ + channel->rx_len = param[2].u.memref.size; + return 0; } @@ -353,6 +357,7 @@ static int setup_dynamic_shmem(struct device *dev, struct scmi_optee_channel *ch shbuf = tee_shm_get_va(channel->tee_shm, 0); memset(shbuf, 0, msg_size); channel->req.msg = shbuf; + channel->rx_len = msg_size; return 0; } @@ -508,7 +513,7 @@ static void scmi_optee_fetch_response(struct scmi_chan_info *cinfo, struct scmi_optee_channel *channel = cinfo->transport_info; if (channel->tee_shm) - msg_fetch_response(channel->req.msg, SCMI_OPTEE_MAX_MSG_SIZE, xfer); + msg_fetch_response(channel->req.msg, channel->rx_len, xfer); else shmem_fetch_response(channel->req.shmem, xfer); } From 4ce7e51dc712f8a006ce6abcc49f788c79287c03 Mon Sep 17 00:00:00 2001 From: Bo Liu Date: Thu, 16 Jun 2022 01:50:52 -0400 Subject: [PATCH 3/3] firmware: arm_scmi: Remove usage of the deprecated ida_simple_xxx API Replace the deprecated ida_simple_{get,remove} with ida_{alloc,free}. Link: https://lore.kernel.org/r/20220616055052.4559-1-liubo03@inspur.com Signed-off-by: Bo Liu [sudeep.holla: Replace ida_alloc_min with ida_alloc as suggested by Cristian] Signed-off-by: Sudeep Holla --- drivers/firmware/arm_scmi/bus.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c index f6fe723ab869..d4e23101448a 100644 --- a/drivers/firmware/arm_scmi/bus.c +++ b/drivers/firmware/arm_scmi/bus.c @@ -181,7 +181,7 @@ scmi_device_create(struct device_node *np, struct device *parent, int protocol, return NULL; } - id = ida_simple_get(&scmi_bus_id, 1, 0, GFP_KERNEL); + id = ida_alloc_min(&scmi_bus_id, 1, GFP_KERNEL); if (id < 0) { kfree_const(scmi_dev->name); kfree(scmi_dev); @@ -204,7 +204,7 @@ scmi_device_create(struct device_node *np, struct device *parent, int protocol, put_dev: kfree_const(scmi_dev->name); put_device(&scmi_dev->dev); - ida_simple_remove(&scmi_bus_id, id); + ida_free(&scmi_bus_id, id); return NULL; } @@ -212,7 +212,7 @@ void scmi_device_destroy(struct scmi_device *scmi_dev) { kfree_const(scmi_dev->name); scmi_handle_put(scmi_dev->handle); - ida_simple_remove(&scmi_bus_id, scmi_dev->id); + ida_free(&scmi_bus_id, scmi_dev->id); device_unregister(&scmi_dev->dev); }