Re: [PATCH] iio: accel: bmc150: clamp the device-reported FIFO frame count

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 13 Jun 2026 02:18:39 -0500
Bryam Vargas via B4 Relay <devnull+hexlabsecurity.proton.me@kernel.org> wrote:

> From: Bryam Vargas <hexlabsecurity@proton.me>
> 
> __bmc150_accel_fifo_flush() copies the number of samples the device
> reports in its hardware FIFO into an on-stack buffer
> 
> 	u16 buffer[BMC150_ACCEL_FIFO_LENGTH * 3];
> 
> which is sized for at most BMC150_ACCEL_FIFO_LENGTH (32) samples. The
> frame count is read from the FIFO_STATUS register and only masked to its
> 7 valid bits:
> 
> 	count = val & 0x7F;
> 
> so it can be 0..127. The only other limit applied to it is the optional
> caller-supplied sample budget:
> 
> 	if (samples && count > samples)
> 		count = samples;
> 
> which does not constrain count on the flush-all path (samples == 0), and
> leaves it well above 32 whenever samples is larger. count samples are
> then transferred into buffer[]:
> 
> 	bmc150_accel_fifo_transfer(data, (u8 *)buffer, count);
> 
> bmc150_accel_fifo_transfer() reads count * 6 bytes through regmap, so a
> malfunctioning, malicious or counterfeit accelerometer (or an attacker
> tampering with the I2C/SPI bus) that reports up to 127 frames writes up
> to 762 bytes into the 192-byte buffer: a stack out-of-bounds write of up
> to 570 bytes that clobbers the stack canary, saved registers and the
> return address.
> 
> Clamp count to BMC150_ACCEL_FIFO_LENGTH, the number of samples buffer[]
> is sized for, before the transfer, mirroring the watermark clamp already
> done in bmc150_accel_set_watermark(). A well-formed flush reports at most
> BMC150_ACCEL_FIFO_LENGTH frames, so legitimate devices are unaffected.
> 
Agreed on the analysis. Thanks for doing this.

I wonder why the field is 7 bits given it can only contain 0 to 32 which only
needs 6 bits...  (Digs in datasheet...) Ah, you can enable just one channel
at a time, however I can't find a statement that doing so increases the number
of frames stored (rather than masking what is read out).  There is plenty
of stuff saying the buffer is full at 32. 

Anyhow, never mind that as we don't really care why the datasheet
is illogical ;)

One small thing inline.


> Fixes: 3bbec9773389 ("iio: bmc150_accel: add support for hardware fifo")
> Cc: stable@vger.kernel.org
> Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
> ---
>  drivers/iio/accel/bmc150-accel-core.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
> index 2398eb7e12cd..dc8a6285cf3d 100644
> --- a/drivers/iio/accel/bmc150-accel-core.c
> +++ b/drivers/iio/accel/bmc150-accel-core.c
> @@ -991,6 +991,8 @@ static int __bmc150_accel_fifo_flush(struct iio_dev *indio_dev,
>  	if (samples && count > samples)
>  		count = samples;
>  
> +	count = min_t(u8, count, BMC150_ACCEL_FIFO_LENGTH);

Why not min()?   I believe that will do the operation as an integer, but
the compiler should be fine spotting that the value that results will
always fit in count.

> +
>  	ret = bmc150_accel_fifo_transfer(data, (u8 *)buffer, count);
>  	if (ret)
>  		return ret;
> 
> ---
> base-commit: 8e65320d91cdc3b241d4b94855c88459b91abf66
> change-id: 20260613-b4-disp-24d6b15f-90f0487ac141
> 
> Best regards,