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

[Hans Verkuil]

On 24/08/2023 09:13, Greg KH wrote:
> On Thu, Aug 24, 2023 at 12:36:30PM +0530, coolrrsh@xxxxxxxxx wrote:
>> From: Rajeshwar R Shinde <coolrrsh@xxxxxxxxx>
>>
>> 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: syzbot+e27f3dbdab04e43b9f73@xxxxxxxxxxxxxxxxxxxxxxxxx
>> Closes: https://lore.kernel.org/all/20230818164522.12806-1-coolrrsh@xxxxxxxxx
>>
>> Link: https://syzkaller.appspot.com/bug?extid=e27f3dbdab04e43b9f73
>>
>> Signed-off-by: Rajeshwar R Shinde <coolrrsh@xxxxxxxxx>
> 
> 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