Re: [PATCH v5 01/13] iio: core: validate file offset in iio_debugfs_write_reg()

[Rodrigo Alencar]

On 26/05/17 07:04PM, sashiko-bot@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] The `*ppos != 0` check introduces a functional regression for sequential writes, masking the true bug which is the inappropriate use of `simple_write_to_buffer()`.
> - [High] Unpatched uninitialized stack memory reads in other IIO drivers caused by the identical anti-pattern.
> --
> 
> commit 7c5ddbc34738b9e84e226b9906348d61e3569bf4
> Author: Rodrigo Alencar <rodrigo.alencar@xxxxxxxxxx>
> 
> iio: core: validate file offset in iio_debugfs_write_reg()
> 
> This patch adds a check to ensure that the file offset is zero in
> iio_debugfs_write_reg() before calling simple_write_to_buffer(). This is
> intended to prevent leaving the start of the buffer uninitialized if a write
> occurs at a non-zero offset.
> 
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > index bd6f4f9f45330..e587aca79b8e7 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -419,7 +419,7 @@ static ssize_t iio_debugfs_write_reg(struct file *file,
> >  	char buf[80];
> >  	int ret;
> >  
> > -	if (count >= sizeof(buf))
> > +	if (count >= sizeof(buf) || *ppos != 0)
> 
> Does this cause a functional regression for sequential writes?
> 
> If a user writes multiple commands to the same open file descriptor, for
> example using (echo "cmd1"; echo "cmd2") > direct_reg_access, the first
> write advances the file position. This means *ppos will be non-zero on the
> second write, causing it to fail with -EINVAL.
> 
> Before simple_write_to_buffer() was introduced, the code used
> copy_from_user() and ignored *ppos, correctly treating every write() call
> as a distinct command starting at the beginning of the buffer.
> 
> Since simple_write_to_buffer() is designed for stateful linear memory buffers
> rather than stateless command endpoints using local stack arrays, would it be
> better to revert the use of simple_write_to_buffer() entirely and use
> copy_from_user() or strndup_user() to parse each write independently?
> 
> Additionally, does this leave a similar pattern unpatched in other IIO
> drivers?
> 
> While this patch addresses the issue in iio_debugfs_write_reg(), the same
> pattern of using simple_write_to_buffer() into a local stack array still
> exists in other locations:
> 
> 1. iio_backend_debugfs_write_reg() in drivers/iio/industrialio-backend.c
> 2. ad3552r_hs_write_data_source() in drivers/iio/dac/ad3552r-hs.c
> 
> Both functions use simple_write_to_buffer() into uninitialized local stack
> arrays without checking *ppos.
> 
> Furthermore, ad9467_chan_test_mode_write() in drivers/iio/adc/ad9467.c
> uses the same pattern. While the buffer is zero-initialized there,
> sequential writes will still be functionally broken.
> 
> >  		return -EINVAL;

This was me trying to fix an issue and maybe creating another.
Probably I would just drop this, and leave this to be handled later.
At least it was good to indicate that such issue exist.

> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260517-ad9910-iio-driver-v5-0-31599c88314a@xxxxxxxxxx?part=1

-- 
Kind regards,

Rodrigo Alencar