linux-kernel - RE: [PATCH] iio: xilinx-ams: Don't include ams_ctrl_channels in scan

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]

Date: Fri, 15 Mar 2024 13:18:42 +0000
From: "O'Griofa, Conall" <conall.ogriofa@....com>
To: Sean Anderson <sean.anderson@...ux.dev>, Jonathan Cameron
	<jic23@...nel.org>
CC: "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, Lars-Peter Clausen <lars@...afoo.de>
Subject: RE: [PATCH] iio: xilinx-ams: Don't include ams_ctrl_channels in
 scan_mask

[AMD Official Use Only - General]

Hi,

I think there was a fix for this issue applied to the version that was running on 5.15 that didn't seem to make it into the upstream driver.
Please see link for reference https://github.com/Xilinx/linux-xlnx/commit/608426961f16ab149b1b699f1c35f7ad244c0720

I think a similar fix to the above patch is may be beneficial?

Cheers,
Conall.

> -----Original Message-----
> From: Sean Anderson <sean.anderson@...ux.dev>
> Sent: Thursday, March 14, 2024 5:30 PM
> To: Jonathan Cameron <jic23@...nel.org>
> Cc: linux-iio@...r.kernel.org; O'Griofa, Conall <conall.ogriofa@....com>;
> linux-arm-kernel@...ts.infradead.org; linux-kernel@...r.kernel.org; Lars-Peter
> Clausen <lars@...afoo.de>
> Subject: Re: [PATCH] iio: xilinx-ams: Don't include ams_ctrl_channels in
> scan_mask
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On 3/14/24 11:48, Jonathan Cameron wrote:
> > On Mon, 11 Mar 2024 12:28:00 -0400
> > Sean Anderson <sean.anderson@...ux.dev> wrote:
> >
> >> ams_enable_channel_sequence constructs a "scan_mask" for all the PS
> >> and PL channels. This works out fine, since scan_index for these
> >> channels is less than 64. However, it also includes the
> >> ams_ctrl_channels, where scan_index is greater than 64, triggering
> >> undefined behavior. Since we don't need these channels anyway, just
> exclude them.
> >>
> >> Fixes: d5c70627a794 ("iio: adc: Add Xilinx AMS driver")
> >> Signed-off-by: Sean Anderson <sean.anderson@...ux.dev>
> >
> > Hi Sean,
> >
> > I'd ideally like to understand why we have channels with such large
> > scan indexes.  Those values should only be used for buffered capture.
> > It feels like they are being abused here.  Can we set them to -1
> > instead and check based on that?
> > For a channel, a scan index of -1 means it can't be captured via the
> > buffered interfaces but only accessed via sysfs reads.
> > I think that's what we have here?
>
> From what I can tell, none of the channels support buffered reads. And we can't
> naïvely convert the scan_index to -1, since that causes sysfs naming conflicts
> (not to mention the compatibility break).
>
> >
> > I just feel like if we leave these as things stand, we will get bitten
> > by similar bugs in the future.  At least with -1 it should be obvious why!
>
> There are just as likely to be bugs confusing the PL/PS subdevices...
>
> FWIW I had no trouble identifying the channels involved with this bug.
>
> --Sean
>
> > Jonathan
> >
> >
> >> ---
> >>
> >>  drivers/iio/adc/xilinx-ams.c | 8 ++++++--
> >>  1 file changed, 6 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/iio/adc/xilinx-ams.c
> >> b/drivers/iio/adc/xilinx-ams.c index a55396c1f8b2..4de7ce598e4d
> >> 100644
> >> --- a/drivers/iio/adc/xilinx-ams.c
> >> +++ b/drivers/iio/adc/xilinx-ams.c
> >> @@ -414,8 +414,12 @@ static void ams_enable_channel_sequence(struct
> >> iio_dev *indio_dev)
> >>
> >>      /* Run calibration of PS & PL as part of the sequence */
> >>      scan_mask = BIT(0) | BIT(AMS_PS_SEQ_MAX);
> >> -    for (i = 0; i < indio_dev->num_channels; i++)
> >> -            scan_mask |= BIT_ULL(indio_dev->channels[i].scan_index);
> >> +    for (i = 0; i < indio_dev->num_channels; i++) {
> >> +            const struct iio_chan_spec *chan =
> >> + &indio_dev->channels[i];
> >> +
> >> +            if (chan->scan_index < AMS_CTRL_SEQ_BASE)
> >> +                    scan_mask |= BIT_ULL(chan->scan_index);
> >> +    }
> >>
> >>      if (ams->ps_base) {
> >>              /* put sysmon in a soft reset to change the sequence */
> >