linux-kernel - Re: [PATCH] media: az6007: fix out-of-bounds in az6007_i2c

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: <c4cf17bb-73cb-4e3b-9e61-8fd682b5e9ee@kernel.org>
Date: Thu, 4 Sep 2025 10:05:24 +0200
From: Hans Verkuil <hverkuil+cisco@...nel.org>
To: Jeongjun Park <aha310510@...il.com>, mchehab@...nel.org
Cc: hverkuil@...all.nl, zhang_shurong@...mail.com,
 linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] media: az6007: fix out-of-bounds in az6007_i2c_xfer()

Hi Jeongjun,

On 21/04/2025 12:55, Jeongjun Park wrote:
> According to the previous commit 1047f9343011 ("media: az6007: 
> Fix null-ptr-deref in az6007_i2c_xfer()"), msgs[i].len is user-controlled.

Does this relate to syzbot reports? If so, please add a reference to that.

As far as I can tell, you can get here only through /dev/i2c-X devices.

> 
> In the previous commit, bounds checking was added because a null-ptr-deref 
> bug occurs when msgs[i].buf and msgs[i].len are set to null. However, this 
> leads to an out-of-bounds vuln for st->data when msgs[i].len is set to a 
> large value.
> 
> Therefore, code to check the maximum value of msgs[i].len needs to be added.
> 
> Fixes: 1047f9343011 ("media: az6007: Fix null-ptr-deref in az6007_i2c_xfer()")
> Signed-off-by: Jeongjun Park <aha310510@...il.com>
> ---
>  drivers/media/usb/dvb-usb-v2/az6007.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/usb/dvb-usb-v2/az6007.c b/drivers/media/usb/dvb-usb-v2/az6007.c
> index 65ef045b74ca..fba1b6c08dc7 100644
> --- a/drivers/media/usb/dvb-usb-v2/az6007.c
> +++ b/drivers/media/usb/dvb-usb-v2/az6007.c
> @@ -788,7 +788,7 @@ static int az6007_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
>  			if (az6007_xfer_debug)
>  				printk(KERN_DEBUG "az6007: I2C W addr=0x%x len=%d\n",
>  				       addr, msgs[i].len);
> -			if (msgs[i].len < 1) {
> +			if (msgs[i].len < 1 || msgs[i].len + 1 > sizeof(st->data)) {
>  				ret = -EIO;
>  				goto err;
>  			}
> @@ -806,7 +806,7 @@ static int az6007_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
>  			if (az6007_xfer_debug)
>  				printk(KERN_DEBUG "az6007: I2C R addr=0x%x len=%d\n",
>  				       addr, msgs[i].len);
> -			if (msgs[i].len < 1) {
> +			if (msgs[i].len < 1 || msgs[i].len + 5 > sizeof(st->data)) {
>  				ret = -EIO;
>  				goto err;
>  			}

I feel uncomfortable about this patch and similar patches that attempt to address this
in various other drivers as well.

It's all rather ad-hoc. E.g. you fix the two places here, but not the case at line 778.

I think the proper fix would be to modify __az6007_read/write and add an argument with the
size of the buffer, then let that function do the check. Rather than manually doing this
check at every call of those functions. Same for similar drivers.

The approach taken in this patch is too fragile.

Regards,

	Hans


> --
>