Re: [PATCH] iio: health: afe4403: Fix oob read in afe4403_read_raw

[Andrew Davis <afd@ti.com>]

On 11/4/22 6:28 AM, Wei Yongjun wrote:
> From: Wei Yongjun <weiyongjun1@huawei.com>
> 
> KASAN report out-of-bounds read as follows:
> 
> BUG: KASAN: global-out-of-bounds in afe4403_read_raw+0x42e/0x4c0 [afe4403]
> Read of size 4 at addr ffffffffc02ac638 by task cat/279
> 
> CPU: 2 PID: 279 Comm: cat Tainted: G                 N
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
> Call Trace:
>   <TASK>
>   afe4403_read_raw+0x42e/0x4c0 [afe4403 141d77410f5466ef049ee2376f5136b77a168de0]
>   iio_read_channel_info+0x249/0x2e0 [industrialio d0627df60a92bbb9630e68c3e2f98d20dac889ef]
>   dev_attr_show+0x4b/0xa0 drivers/base/core.c:2195
>   sysfs_kf_seq_show+0x1ec/0x390 fs/sysfs/file.c:59
>   seq_read_iter+0x48d/0x10b0 fs/seq_file.c:230
>   kernfs_fop_read_iter+0x4e6/0x710 fs/kernfs/file.c:275
>   call_read_iter include/linux/fs.h:2153 [inline]
>   new_sync_read fs/read_write.c:389 [inline]
>   vfs_read+0x5f2/0x890 fs/read_write.c:470
>   ksys_read+0x106/0x220 fs/read_write.c:613
>   do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>   do_syscall_64+0x38/0xa0 arch/x86/entry/common.c:80
>   entry_SYSCALL_64_after_hwframe+0x72/0xdc
> RIP: 0033:0x7fd8611cf992
>   </TASK>
> 
> The buggy address belongs to the variable:
>   afe4403_channel_leds+0x18/0xffffffffffffe9e0 [afe4403]
> 
> This issue can be reproduced by singe command:
> 
>   $ cat /sys/bus/spi/devices/spi0.0/iio\:device0/in_intensity6_raw
> 
> The array size of afe4403_channel_leds is less than channels, so access
> with chan->address cause OOB read in afe4403_read_raw. Fix it by moving
> access before use it.
> 
> Fixes: b36e8257641a ("iio: health/afe440x: Use regmap fields")
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> ---
>   drivers/iio/health/afe4403.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/health/afe4403.c b/drivers/iio/health/afe4403.c
> index 3bb4028c5d74..14213a48e349 100644
> --- a/drivers/iio/health/afe4403.c
> +++ b/drivers/iio/health/afe4403.c
> @@ -246,7 +246,7 @@ static int afe4403_read_raw(struct iio_dev *indio_dev,
>   {
>   	struct afe4403_data *afe = iio_priv(indio_dev);
>   	unsigned int reg = afe4403_channel_values[chan->address];

Good find and the fix does look valid, but can we also move this
access for 'reg' to right before we use it also? Just for consistency.

Same for the 'afe4403_channel_leds' access below in afe4403_write_raw(),
and same for value_reg in your patch for afe4404. Then both patches can have:

Acked-by: Andrew Davis <afd@ti.com>

> -	unsigned int field = afe4403_channel_leds[chan->address];
> +	unsigned int field;
>   	int ret;
>   
>   	switch (chan->type) {
> @@ -262,6 +262,7 @@ static int afe4403_read_raw(struct iio_dev *indio_dev,
>   	case IIO_CURRENT:
>   		switch (mask) {
>   		case IIO_CHAN_INFO_RAW:
> +			field = afe4403_channel_leds[chan->address];
>   			ret = regmap_field_read(afe->fields[field], val);
>   			if (ret)
>   				return ret;