linux-kernel - Re: [PATCH v3] iio: light: as73211: fix channel handling in only-color triggered buffer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <D6BITV4LNT71.O2XWAQNX16MV@gmail.com>
Date: Sat, 14 Dec 2024 16:16:20 +0100
From: "Javier Carrasco" <javier.carrasco.cruz@...il.com>
To: "Jonathan Cameron" <jic23@...nel.org>
Cc: "Lars-Peter Clausen" <lars@...afoo.de>, "Christian Eggers"
 <ceggers@...i.de>, "Jonathan Cameron" <Jonathan.Cameron@...wei.com>,
 <linux-iio@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <stable@...r.kernel.org>
Subject: Re: [PATCH v3] iio: light: as73211: fix channel handling in
 only-color triggered buffer

On Sat Dec 14, 2024 at 3:22 PM CET, Jonathan Cameron wrote:
> On Thu, 12 Dec 2024 18:56:32 +0100
> Javier Carrasco <javier.carrasco.cruz@...il.com> 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.
> >
> > Add available_scan_masks for all channels and only-color channels to let
> > the IIO core demux and repack the enabled channels.
> >
> > Cc: stable@...r.kernel.org
> > Fixes: 403e5586b52e ("iio: light: as73211: New driver")
> > Tested-by: Christian Eggers <ceggers@...i.de>
> > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@...il.com>
> > ---
> > This issue was found after attempting to make the same mistake for
> > a driver I maintain, which was fortunately spotted by Jonathan [1].
> >
> > Keeping old sensor values if the channel configuration changes is known
> > and not considered an issue, which is also mentioned in [1], so it has
> > not been addressed by this series. That keeps most of the drivers out
> > of the way because they store the scan element in iio private data,
> > which is kzalloc() allocated.
> >
> > This series only addresses cases where uninitialized i.e. unknown data
> > is pushed to the userspace, either due to holes in structs or
> > uninitialized struct members/array elements.
> >
> > While analyzing involved functions, I found and fixed some triviality
> > (wrong function name) in the documentation of iio_dev_opaque.
> >
> > Link: https://lore.kernel.org/linux-iio/20241123151634.303aa860@jic23-huawei/ [1]
> > ---
> > Changes in v3:
> > - as73211.c: add available_scan_masks for all channels and only-color
> >   channels to let the IIO core demux and repack the enabled channels.
> > - Link to v2: https://lore.kernel.org/r/20241204-iio_memset_scan_holes-v2-0-3f941592a76d@gmail.com
> >
> > Changes in v2:
> > - as73211.c: shift channels if no temperature is available and
> >   initialize chan[3] to zero.
> > - Link to v1: https://lore.kernel.org/r/20241125-iio_memset_scan_holes-v1-0-0cb6e98d895c@gmail.com
> > ---
> >  drivers/iio/light/as73211.c | 24 ++++++++++++++++++++----
> >  1 file changed, 20 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c
> > index be0068081ebb..4be2e349a068 100644
> > --- a/drivers/iio/light/as73211.c
> > +++ b/drivers/iio/light/as73211.c
> > @@ -177,6 +177,12 @@ struct as73211_data {
> >  	BIT(AS73211_SCAN_INDEX_TEMP) | \
> >  	AS73211_SCAN_MASK_COLOR)
> >
> > +static const unsigned long as73211_scan_masks[] = {
> > +	AS73211_SCAN_MASK_ALL,
> > +	AS73211_SCAN_MASK_COLOR,
>
> I probably mislead you on this :(
> Needs to be the other way around as the core code starts at first
> entry whilst trying to find a mask that is a superset of what is turned on.
> here that means it will always use the first one.
> See iio_scan_mask_match() - strict isn't set int this case.
>

Thanks for your feedback. I copied your suggestion, but it was my fault
that I did not make sure it was right for the intended behavior.

I will send a new version with that fixed soon.

Best regards,
Javier Carrasco