On 26/05/17 07:04PM, [email protected] 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 <[email protected]> > > 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/[email protected]?part=1 -- Kind regards, Rodrigo Alencar
