2023-08-24 09:19:14

by Hans Verkuil

[permalink] [raw]
Subject: Re: [PATCH v2] UBSAN: shift-out-of-bounds in set_flicker

On 24/08/2023 09:13, Greg KH wrote:
> On Thu, Aug 24, 2023 at 12:36:30PM +0530, [email protected] wrote:
>> From: Rajeshwar R Shinde <[email protected]>
>>
>> Syzkaller reported the following issue:
>>
>> UBSAN: shift-out-of-bounds in drivers/media/usb/gspca/cpia1.c:1031:27
>> shift exponent 245 is too large for 32-bit type 'int'
>>
>> shift-out-of-bounds error was triggered when variable
>> 'sd->params.exposure.gain' is greater than the number of bits of int.
>> When the variable 'currentexp' is left shifted beyond 31 bits then
>> the error is produced. Therefore added the conditional expression to
>> verify valid range.
>>
>> Tested via syzbot.
>>
>> Reported-by: [email protected]
>> Closes: https://lore.kernel.org/all/[email protected]
>>
>> Link: https://syzkaller.appspot.com/bug?extid=e27f3dbdab04e43b9f73
>>
>> Signed-off-by: Rajeshwar R Shinde <[email protected]>
>
> Please do not have blank lines beween these tags.
>
> You also have trailing whitespace in your changelog text :(

And please mention the driver name in your subject! E.g. something like:

"media: gspca/cpia1: UBSAN: shift-out-of-bounds in set_flicker"

That way I can actually know that this is a patch for a driver that I
maintain.

Regards,

Hans

>
>> ---
>> v1->v2
>> changed the patch
>> changed commit message and tested with checkpatch
>>
>> ---
>> drivers/media/usb/gspca/cpia1.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/media/usb/gspca/cpia1.c b/drivers/media/usb/gspca/cpia1.c
>> index 46ed95483e22..dafc522d5e7b 100644
>> --- a/drivers/media/usb/gspca/cpia1.c
>> +++ b/drivers/media/usb/gspca/cpia1.c
>> @@ -1028,6 +1028,8 @@ static int set_flicker(struct gspca_dev *gspca_dev, int on, int apply)
>> sd->params.exposure.expMode = 2;
>> sd->exposure_status = EXPOSURE_NORMAL;
>> }
>> + if (sd->params.exposure.gain > 31)
>> + return -1;
>
> Do not make up error codes, please return a valid one and not a random
> negative number. Unless -1 is a valid value for this function?
>
> thanks,
>
> greg k-h