Re: [PATCH v5 2/2] iio: adc: adding support for PAC193x

From: <Marius.Cristea@microchip.com>
To: <jic23@kernel.org>, <Conor.Dooley@microchip.com>
Cc: <linux@roeck-us.net>, <jdelvare@suse.com>,
	<linux-iio@vger.kernel.org>, <devicetree@vger.kernel.org>,
	<lars@metafoo.de>, <linux-kernel@vger.kernel.org>,
	<linux-hwmon@vger.kernel.org>,
	<krzysztof.kozlowski+dt@linaro.org>, <robh+dt@kernel.org>,
	<conor+dt@kernel.org>
Subject: Re: [PATCH v5 2/2] iio: adc: adding support for PAC193x
Date: Fri, 5 Apr 2024 12:53:38 +0000	[thread overview]
Message-ID: <e432bddc16952d9144ccf6da5a54b88e3171b947.camel@microchip.com> (raw)
In-Reply-To: <20240405-embellish-bonnet-ab5f10560d93@wendy>

Hi Conor,

    Thanks for reporting the bug. I have detect it and I'm already
working on a patch for it.

Thanks,
Marius

On Fri, 2024-04-05 at 13:41 +0100, Conor Dooley wrote:
> On Sat, Feb 24, 2024 at 07:15:59PM +0000, Jonathan Cameron wrote:
> > On Thu, 22 Feb 2024 18:42:06 +0200
> > <marius.cristea@microchip.com> wrote:
> > 
> > > From: Marius Cristea <marius.cristea@microchip.com>
> > > 
> > > This is the iio driver for Microchip
> > > PAC193X series of Power Monitor with Accumulator chip family.
> > > 
> > > Signed-off-by: Marius Cristea <marius.cristea@microchip.com>
> > So I had a few comments on this, but nothing that can't be cleaned
> > up later.
> > + I'll fix the thing the bots didn't like on the bindings.
> > 
> > Series applied to the togreg branch of iio.git and pushed out
> > as testing for 0-day to take a look at it.
> 
> I tested this out on v6.9-rc2 and prompted a backtrace when collectd
> started running:
>         ------------[ cut here ]------------
>         UBSAN: array-index-out-of-bounds in
> /home/conor/stuff/linux/drivers/iio/adc/pac1934.c:857:25
>         index 7 is out of range for type 'u32 [4]'
>         CPU: 1 PID: 179 Comm: iiod Not tainted 6.9.0-rc2-dirty #1
>         Hardware name: Microchip PolarFire-SoC Icicle Kit (DT)
>         Call Trace:
>         [<ffffffff80006bba>] dump_backtrace+0x28/0x30
>         [<ffffffff80bd67d8>] show_stack+0x38/0x44
>         [<ffffffff80be7820>] dump_stack_lvl+0x6e/0x9a
>         [<ffffffff80be7864>] dump_stack+0x18/0x20
>         [<ffffffff80be1452>] ubsan_epilogue+0x10/0x46
>         [<ffffffff80615358>] __ubsan_handle_out_of_bounds+0x6a/0x78
>         [<ffffffff80981f3a>] pac1934_read_raw+0x20c/0x34c
>         [<ffffffff80977c4c>] iio_read_channel_info+0x5c/0xbe
>         [<ffffffff8073516e>] dev_attr_show+0x1c/0x4a
>         [<ffffffff80387292>] sysfs_kf_seq_show+0x80/0xcc
>         [<ffffffff80385b12>] kernfs_seq_show+0x3c/0x4a
>         [<ffffffff8031e3d8>] seq_read_iter+0x136/0x2e4
>         [<ffffffff80385cde>] kernfs_fop_read_iter+0x38/0x16a
>         [<ffffffff802e904a>] vfs_read+0x1be/0x2ba
>         [<ffffffff802e9c48>] ksys_read+0x64/0xd2
>         [<ffffffff802e9cd6>] __riscv_sys_read+0x20/0x28
>         [<ffffffff80be838a>] do_trap_ecall_u+0xee/0x204
>         [<ffffffff80bf57d0>] ret_from_exception+0x0/0x64
>         ---[ end trace ]---
> 
> The device itself only has 4 channels, but in sysfs there are "fake"
> channels for the average voltages and currents too. UBSAN points at:
>         case PAC1934_VSENSE_AVG_4_ADDR:
>                 *val = PAC1934_MAX_VSENSE_RSHIFTED_BY_16B;
>                 if (chan->scan_type.sign == 'u')
>                         *val2 = info->shunts[channel];
>                 else
>                         *val2 = info->shunts[channel] >> 1;
>                 return IIO_VAL_FRACTIONAL;
> 
> And info->shunts is only valid for the 4 real channels, so I guess
> the
> averaged channels probably need to do a [channel - 4] or similar. I
> dunno if that relation between averaged channels and their "real"
> counterparts is fixed or if different pac devices need different
> values,
> but on my system here that would work.
> 
> I do quite like that UBSAN points out the line in question :)
> 
> Cheers,
> Conor.