linux-kernel - Re: [PATCH] iio: adc: Revoke valid channel for error path

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <e8b24cf22c87e5b5ce0cc8919eca79f6e60ab6e3.camel@gmail.com>
Date: Thu, 17 Apr 2025 15:02:16 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: Gabriel Shahrouzi <gshahrouzi@...il.com>
Cc: gregkh@...uxfoundation.org, jic23@...nel.org, lars@...afoo.de, 
	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-staging@...ts.linux.dev, Michael.Hennerich@...log.com, 
	sonic.zhang@...log.com, vapier@...too.org, skhan@...uxfoundation.org, 
	kernelmentees@...ts.linuxfoundation.org, stable@...r.kernel.org
Subject: Re: [PATCH] iio: adc: Revoke valid channel for error path

On Thu, 2025-04-17 at 08:53 -0400, Gabriel Shahrouzi wrote:
> On Thu, Apr 17, 2025 at 6:06 AM Nuno Sá <noname.nuno@...il.com> wrote:
> > 
> > On Tue, 2025-04-15 at 14:20 -0400, Gabriel Shahrouzi wrote:
> > > According to the datasheet on page 9 under the channel selection table,
> > > all devices (AD7816/7/8) are able to use the channel marked as 7. This
> > > channel is used for diagnostic purposes by routing the internal 1.23V
> > > bandgap source through the MUX to the input of the ADC.
> > > 
> > > Replace checking for string equality with checking for the same chip ID
> > > to reduce time complexity.
> > > 
> > > Group invalid channels for all devices together because they are
> > > processed the same way.
> > > 
> > > Fixes: 7924425db04a ("staging: iio: adc: new driver for AD7816 devices")
> > > Cc: stable@...r.kernel.org
> > > Signed-off-by: Gabriel Shahrouzi <gshahrouzi@...il.com>
> > > ---
> > >  drivers/staging/iio/adc/ad7816.c | 15 +++++----------
> > >  1 file changed, 5 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/staging/iio/adc/ad7816.c
> > > b/drivers/staging/iio/adc/ad7816.c
> > > index 6c14d7bcdd675..d880fe0257697 100644
> > > --- a/drivers/staging/iio/adc/ad7816.c
> > > +++ b/drivers/staging/iio/adc/ad7816.c
> > > @@ -186,17 +186,12 @@ static ssize_t ad7816_store_channel(struct device
> > > *dev,
> > >       if (ret)
> > >               return ret;
> > > 
> > > -     if (data > AD7816_CS_MAX && data != AD7816_CS_MASK) {
> > > -             dev_err(&chip->spi_dev->dev, "Invalid channel id %lu for
> > > %s.\n",
> > > -                     data, indio_dev->name);
> > > -             return -EINVAL;
> > > -     } else if (strcmp(indio_dev->name, "ad7818") == 0 && data > 1) {
> > > -             dev_err(&chip->spi_dev->dev,
> > > -                     "Invalid channel id %lu for ad7818.\n", data);
> > > -             return -EINVAL;
> > > -     } else if (strcmp(indio_dev->name, "ad7816") == 0 && data > 0) {
> > > +     if (data != AD7816_CS_MASK &&
> > > +         (data > AD7816_CS_MAX ||
> > > +         (chip->id == ID_AD7818 && data > 1) ||
> > > +         (chip->id == ID_AD7816 && data > 0))) {
> > >               dev_err(&chip->spi_dev->dev,
> > > -                     "Invalid channel id %lu for ad7816.\n", data);
> > > +                     "Invalid channel id %lu for %s.\n", data, indio_dev-
> > > > name);
> > >               return -EINVAL;
> > >       }
> > 
> > Hmm, maybe I'm missing something but the code just looks the same as before
> > (from a functionality point of view)? I'm really not seeing any fix...
> I might have to change it for readability. From my understanding, if
> channel 7 is selected (AD7816_CS_MASK), it never enters the error path
> whereas in the old code, if the chip were either ad7816 or ad7818, it would
> end up returning an error because it skips all channels above either 0
> or 1.

Ahh, right!

One good refactor is to add a chip_info struct (renaming the existing one) with
let's say a name and max_channels. Then, the condition could be reduced to:

if (data > st->chip->max_channel && data != AD7816_CS_MASK {
	dev_err(...);
	return -EINVAL;
}

Being this in staging, I guess we don't care much about having the fix as the
first patch to make it easier to backport.

- Nuno Sá

> 
> > 
> > Having said the above, not sure if grouping helps with readability. But I do
> > agree with moving from string comparison to use chip->id. And we also have
> > redundants 'else'
> > 
> > - Nuno Sá
> >