Re: [PATCH] platform/chrome: sensorhub: Fix memory overread in ring handler

Hi Tzung-Bi,


On Mon, Jun 29, 2026 at 6:24 PM Tzung-Bi Shih <[email protected]> wrote:
>
> `max_response` and `sensor_num` are read from different EC commands:
>
> - `max_response` is from cros_ec_get_proto_info().
>     ec_dev->max_response = info->max_response_packet_size -
>                                     sizeof(struct ec_host_response);
>
> - `sensor_num` is from cros_ec_get_sensor_count().
>     sensor_num = cros_ec_get_sensor_count(ec);
>
> With a malfunctioning EC firmware, it is possible that the `msg->insize`
> (i.e., `fifo_info_length` in the context) could be clamped in
> cros_ec_cmd_xfer() because `msg->insize` is greater than `max_response`.
>
>     int fifo_info_length =
>             sizeof(struct ec_response_motion_sense_fifo_info) +
>             sizeof(u16) * sensorhub->sensor_num;
>
> This means the number of read bytes could be less than expected.  As a
> result, the subsequent memcpy() in cros_ec_sensorhub_ring_handler()
> overreads the `resp->fifo_info` buffer.
>
> Check the return value of cros_ec_cmd_xfer_status() and clamp the length
> passed to memcpy() to the actual number of bytes read.
>
> Fixes: 145d59baff59 ("platform/chrome: cros_ec_sensorhub: Add FIFO support")
> Signed-off-by: Tzung-Bi Shih <[email protected]>
> ---
>  drivers/platform/chrome/cros_ec_sensorhub_ring.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)

Thanks for the patch. Please see my comments inline.

>
> diff --git a/drivers/platform/chrome/cros_ec_sensorhub_ring.c b/drivers/platform/chrome/cros_ec_sensorhub_ring.c
> index 64e9615ed6f4..4b4adc38485c 100644
> --- a/drivers/platform/chrome/cros_ec_sensorhub_ring.c
> +++ b/drivers/platform/chrome/cros_ec_sensorhub_ring.c
> @@ -825,11 +825,19 @@ static void cros_ec_sensorhub_ring_handler(struct cros_ec_sensorhub *sensorhub)
>                 sensorhub->msg->outsize = 1;
>                 sensorhub->msg->insize = fifo_info_length;
>
> -               if (cros_ec_cmd_xfer_status(ec->ec_dev, sensorhub->msg) < 0)
> +               ret = cros_ec_cmd_xfer_status(ec->ec_dev, sensorhub->msg);
> +               if (ret < 0)

I think it would be much safer to just error out if ret != fifo_info_length:
 - The data read here is used later in the code. Filling the array
with zeroes would cause the code to act on some data that doesn't
match the real hardware state, potentially leading to other buggy
behaviors,
 - We silently mask bad EC behavior.

We should probably also add a rate-limited print here to leave a trace
in the logs.

Best,
Tomasz

>                         goto error;
>
>                 memcpy(fifo_info, &sensorhub->resp->fifo_info,
> -                      fifo_info_length);
> +                      min(fifo_info_length, ret));
> +
> +               if (ret < fifo_info_length) {
> +                       dev_info_ratelimited(sensorhub->dev,
> +                                            "Read less FIFO info: size %d - expected %d\n",
> +                                            ret, fifo_info_length);
> +                       memset((u8 *)fifo_info + ret, 0, fifo_info_length - ret);
> +               }
>
>                 /*
>                  * Update collection time, will not be as precise as the
> --
> 2.55.0.rc0.799.gd6f94ed593-goog
>