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: <CAO9qdTHJ9GrbqjtMKzgDhy+bvEmDc+Sn3VosYxuq5hJ9Z20-bA@mail.gmail.com>
Date: Thu, 4 Sep 2025 20:07:44 +0900
From: Jeongjun Park <aha310510@...il.com>
To: Hans Verkuil <hverkuil+cisco@...nel.org>
Cc: mchehab@...nel.org, 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 Hans,

Hans Verkuil <hverkuil+cisco@...nel.org> wrote:
>
> 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.
>

Sorry, I seem to have forgotten to include the reported-by tag when
sending the email. I'll add it in the next patch.


> >
> > 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.
>

You're right. Looking at it again, it seems more appropriate to fix
__az6007_read/write.

https://lore.kernel.org/all/20250421125244.85640-1-aha310510@gmail.com/

I remember patching vulnerabilities I found in other drivers using the
method Hans suggested. Is this the correct way to patch them?

> Regards,
>
>         Hans
>
>
> > --
> >
>

Regards,
Jeongjun Park