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

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

[Bryam Vargas]

A couple of things that belong with the patch I just sent: the reproducer
I used, and a heads-up about an in-flight series on this file.

Reproducer
==========
I reproduced the out-of-bounds write with an in-kernel test that drives the
__bmc150_accel_fifo_flush() buffer geometry verbatim under KASAN
(CONFIG_KASAN_STACK=y), with the device-supplied frame count:

  count=127, samples=0 (flush-all), no clamp:
    BUG: KASAN: stack-out-of-bounds in ...fifo_flush
    Write of size 762 ... 'buffer'   (the 192-byte buffer)
  with the patch (clamp to BMC150_ACCEL_FIFO_LENGTH): the transfer is
    bounded to 32 samples, no KASAN report.
  a well-formed (<= 32 sample) flush is unaffected, no KASAN report.

The full 762-byte write -- a 570-byte overflow past buffer[] that smashes the
stack canary and the return address -- reproduces the same way under userspace
AddressSanitizer on -m32 and -m64. The FIFO frame count is read over I2C/SPI
from the accelerometer, so triggering this requires a malicious or defective
device, or tampering on the bus. No CVE has been assigned.

In-flight series on this file
=============================
There is an in-flight cleanup from Gabriel Rondon converting this driver's
locking to guard(mutex):

  [PATCH v3] iio: accel: bmc150: use guard(mutex) for mutex handling
  https://lore.kernel.org/all/20260525110130.61284-1-grondon@gmail.com/

That series only rewrites the mutex_lock()/mutex_unlock() pairs -- including
the bmc150_accel_fifo_flush() wrapper -- and does not touch the count handling
in __bmc150_accel_fifo_flush(), so this fix is independent of it and the two
apply cleanly together. This patch is against mainline; if the guard(mutex)
series lands first it needs only a context-line refresh.

Thanks,
Bryam

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

[Bryam Vargas via B4 Relay]

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.

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);
+
 	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,
-- 
Bryam Vargas <hexlabsecurity@proton.me>

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

[Jonathan Cameron]

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,