linux-kernel - Re: [PATCH v2] UBSAN: shift-out-of-bounds in set

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <2b05ceb2-5d95-63bf-fba8-16e68c64089b@xs4all.nl>
Date:   Thu, 24 Aug 2023 09:19:22 +0200
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Greg KH <greg@...ah.com>, coolrrsh@...il.com
Cc:     mchehab@...nel.org, slark_xiao@....com,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-kernel-mentees@...ts.linuxfoundation.org,
        syzbot+e27f3dbdab04e43b9f73@...kaller.appspotmail.com
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, coolrrsh@...il.com wrote:
>> From: Rajeshwar R Shinde <coolrrsh@...il.com>
>>
>> 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@...kaller.appspotmail.com
>> Closes: https://lore.kernel.org/all/20230818164522.12806-1-coolrrsh@gmail.com
>>
>> Link: https://syzkaller.appspot.com/bug?extid=e27f3dbdab04e43b9f73
>>
>> Signed-off-by: Rajeshwar R Shinde <coolrrsh@...il.com>
> 
> 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