| Message ID | 20241204-iio_memset_scan_holes-v2-2-3f941592a76d@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: fix information leaks in triggered buffers | expand |
On Wednesday, 4 December 2024, 00:55:32 CET, Javier Carrasco wrote: > The channel index is off by one unit if AS73211_SCAN_MASK_ALL is not > set (optimized path for color channel readings), and it must be shifted > instead of leaving an empty channel for the temperature when it is off. > > Once the channel index is fixed, the uninitialized channel must be set > to zero to avoid pushing uninitialized data. > > Cc: stable@vger.kernel.org > Fixes: 403e5586b52e ("iio: light: as73211: New driver") > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> > --- > drivers/iio/light/as73211.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c > index be0068081ebb..2d45dfeda406 100644 > --- a/drivers/iio/light/as73211.c > +++ b/drivers/iio/light/as73211.c > @@ -672,9 +672,12 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p) > > /* AS73211 starts reading at address 2 */ > ret = i2c_master_recv(data->client, > - (char *)&scan.chan[1], 3 * sizeof(scan.chan[1])); > + (char *)&scan.chan[0], 3 * sizeof(scan.chan[0])); > if (ret < 0) > goto done; > + > + /* Avoid pushing uninitialized data */ > + scan.chan[3] = 0; > } > > if (data_result) { > @@ -682,9 +685,15 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p) > * Saturate all channels (in case of overflows). Temperature channel > * is not affected by overflows. > */ > - scan.chan[1] = cpu_to_le16(U16_MAX); > - scan.chan[2] = cpu_to_le16(U16_MAX); > - scan.chan[3] = cpu_to_le16(U16_MAX); > + if (*indio_dev->active_scan_mask == AS73211_SCAN_MASK_ALL) { > + scan.chan[1] = cpu_to_le16(U16_MAX); > + scan.chan[2] = cpu_to_le16(U16_MAX); > + scan.chan[3] = cpu_to_le16(U16_MAX); > + } else { > + scan.chan[0] = cpu_to_le16(U16_MAX); > + scan.chan[1] = cpu_to_le16(U16_MAX); > + scan.chan[2] = cpu_to_le16(U16_MAX); > + } > } > > iio_push_to_buffers_with_timestamp(indio_dev, &scan, iio_get_time_ns(indio_dev)); > > With this change, having only X, Y and Z in the scan_mask (without the temperature channel) works fine. But it looks that there is still another problem if a single color channel (e.g. X) is omitted from the scan mask (which probably wouldn't make much sense in practice). If I am right, the layout of scan.chan[] is also wrong for that case, so e.g. if omitting X, the application will get the X values where it expects the temperature value (which isn't read from the hardware at all). Does it make sense to write a follow-up patch for this? I fear that taking all possible combinations into account could make the driver more complicated than necessary. Or is there a good example how to handle such combinations? Tested-by: Christian Eggers <ceggers@arri.de>
On Wed, 4 Dec 2024 17:20:47 +0100 Christian Eggers <ceggers@arri.de> wrote: > On Wednesday, 4 December 2024, 00:55:32 CET, Javier Carrasco wrote: > > The channel index is off by one unit if AS73211_SCAN_MASK_ALL is not > > set (optimized path for color channel readings), and it must be shifted > > instead of leaving an empty channel for the temperature when it is off. > > > > Once the channel index is fixed, the uninitialized channel must be set > > to zero to avoid pushing uninitialized data. > > > > Cc: stable@vger.kernel.org > > Fixes: 403e5586b52e ("iio: light: as73211: New driver") > > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> > > --- > > drivers/iio/light/as73211.c | 17 +++++++++++++---- > > 1 file changed, 13 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c > > index be0068081ebb..2d45dfeda406 100644 > > --- a/drivers/iio/light/as73211.c > > +++ b/drivers/iio/light/as73211.c > > @@ -672,9 +672,12 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p) > > > > /* AS73211 starts reading at address 2 */ > > ret = i2c_master_recv(data->client, > > - (char *)&scan.chan[1], 3 * sizeof(scan.chan[1])); > > + (char *)&scan.chan[0], 3 * sizeof(scan.chan[0])); > > if (ret < 0) > > goto done; > > + > > + /* Avoid pushing uninitialized data */ > > + scan.chan[3] = 0; > > } > > > > if (data_result) { > > @@ -682,9 +685,15 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p) > > * Saturate all channels (in case of overflows). Temperature channel > > * is not affected by overflows. > > */ > > - scan.chan[1] = cpu_to_le16(U16_MAX); > > - scan.chan[2] = cpu_to_le16(U16_MAX); > > - scan.chan[3] = cpu_to_le16(U16_MAX); > > + if (*indio_dev->active_scan_mask == AS73211_SCAN_MASK_ALL) { > > + scan.chan[1] = cpu_to_le16(U16_MAX); > > + scan.chan[2] = cpu_to_le16(U16_MAX); > > + scan.chan[3] = cpu_to_le16(U16_MAX); > > + } else { > > + scan.chan[0] = cpu_to_le16(U16_MAX); > > + scan.chan[1] = cpu_to_le16(U16_MAX); > > + scan.chan[2] = cpu_to_le16(U16_MAX); > > + } > > } > > > > iio_push_to_buffers_with_timestamp(indio_dev, &scan, iio_get_time_ns(indio_dev)); > > > > > > With this change, having only X, Y and Z in the scan_mask (without the > temperature channel) works fine. > > But it looks that there is still another problem if a single color channel > (e.g. X) is omitted from the scan mask (which probably wouldn't make much > sense in practice). If I am right, the layout of scan.chan[] is also wrong for > that case, so e.g. if omitting X, the application will get the X values where > it expects the temperature value (which isn't read from the hardware at all). > > Does it make sense to write a follow-up patch for this? I fear that taking all > possible combinations into account could make the driver more complicated than > necessary. Or is there a good example how to handle such combinations? > Good spot. I'd fallen for assuming a driver worked the way I thought it would and not checked everything necessary was there. Hmm. This is a bit odd. Driver seems to be written with assumption that the IIO core is doing demux. That doesn't work unless available_scan_masks is set. Make that { AS73211_SCAN_MASK_ALL, AS73211_SCAN_MASK_COLOR, 0, }; And then if you enable fewer channels, the IIO core will still enable one of the sets in available_scan_masks and then do the relevant data manipulation to repack as necessary. I'll not pick this patch up as it makes sense to fix both issues together. Thanks Jonathan > > Tested-by: Christian Eggers <ceggers@arri.de> > > > >
diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c index be0068081ebb..2d45dfeda406 100644 --- a/drivers/iio/light/as73211.c +++ b/drivers/iio/light/as73211.c @@ -672,9 +672,12 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p) /* AS73211 starts reading at address 2 */ ret = i2c_master_recv(data->client, - (char *)&scan.chan[1], 3 * sizeof(scan.chan[1])); + (char *)&scan.chan[0], 3 * sizeof(scan.chan[0])); if (ret < 0) goto done; + + /* Avoid pushing uninitialized data */ + scan.chan[3] = 0; } if (data_result) { @@ -682,9 +685,15 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p) * Saturate all channels (in case of overflows). Temperature channel * is not affected by overflows. */ - scan.chan[1] = cpu_to_le16(U16_MAX); - scan.chan[2] = cpu_to_le16(U16_MAX); - scan.chan[3] = cpu_to_le16(U16_MAX); + if (*indio_dev->active_scan_mask == AS73211_SCAN_MASK_ALL) { + scan.chan[1] = cpu_to_le16(U16_MAX); + scan.chan[2] = cpu_to_le16(U16_MAX); + scan.chan[3] = cpu_to_le16(U16_MAX); + } else { + scan.chan[0] = cpu_to_le16(U16_MAX); + scan.chan[1] = cpu_to_le16(U16_MAX); + scan.chan[2] = cpu_to_le16(U16_MAX); + } } iio_push_to_buffers_with_timestamp(indio_dev, &scan, iio_get_time_ns(indio_dev));
The channel index is off by one unit if AS73211_SCAN_MASK_ALL is not set (optimized path for color channel readings), and it must be shifted instead of leaving an empty channel for the temperature when it is off. Once the channel index is fixed, the uninitialized channel must be set to zero to avoid pushing uninitialized data. Cc: stable@vger.kernel.org Fixes: 403e5586b52e ("iio: light: as73211: New driver") Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> --- drivers/iio/light/as73211.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)