[PATCH] media: az6007: fix out-of-bounds in az6007_i2c_xfer()

[PATCH] media: az6007: fix out-of-bounds in az6007_i2c_xfer()

[Jeongjun Park]

According to the previous commit 1047f9343011 ("media: az6007: 
Fix null-ptr-deref in az6007_i2c_xfer()"), msgs[i].len is user-controlled.

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@gmail.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;
 			}
--

Re: [PATCH] media: az6007: fix out-of-bounds in az6007_i2c_xfer()

[Hans Verkuil]

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


> --
> 

Re: [PATCH] media: az6007: fix out-of-bounds in az6007_i2c_xfer()

[Jeongjun Park]

Hi Hans,

Hans Verkuil <hverkuil+cisco@kernel.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@gmail.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

Re: [PATCH] media: az6007: fix out-of-bounds in az6007_i2c_xfer()

[Hans Verkuil]

On 04/09/2025 13:07, Jeongjun Park wrote:
> Hi Hans,
> 
> Hans Verkuil <hverkuil+cisco@kernel.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@gmail.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?

Yes, that's probably the correct way.

Your patch for drivers/media/usb/dvb-usb/az6027.c has the same problem as this patch
for the az6007, so if you can provide an updated version of that patch as well, then
that will be great.

It's actually more complicated: az6007_i2c_xfer also has code that writes to st->data
before calling __az6007_write:

	for (j = 0; j < len; j++)
		st->data[j] = msgs[i].buf[j + 1];

So there we do need a check, we can't rely on __az6007_write to do that.

It's really messy code, perhaps __az6007_write/read should just receive the st pointer,
and the msgs[i].buf+offset pointer, and have it do the copying and checking against
sizeof(st->data). I think that's what dtv5100_i2c_msg does, so perhaps this is a good
template to use.

Regards,

	Hans

> 
>> Regards,
>>
>>         Hans
>>
>>
>>> --
>>>
>>
> 
> Regards,
> Jeongjun Park