UBSAN: shift-out-of-bounds in drivers/media/usb/uvc/uvc_ctrl.c:781

UBSAN: shift-out-of-bounds in drivers/media/usb/uvc/uvc_ctrl.c:781

[Bart Van Assche]

Hi,

This morning I installed a debug build of kernel v5.8.1 on my laptop.
The complaint shown below appeared in the kernel log. Is this a known
issue?

Thanks,

Bart.

================================================================================
UBSAN: shift-out-of-bounds in drivers/media/usb/uvc/uvc_ctrl.c:781:13
shift exponent -7 is negative
CPU: 5 PID: 8871 Comm: V4L2CaptureThre Tainted: G           O      5.8.1-bva+ #2
Hardware name: Dell Inc. Precision 5520/0R6JFH, BIOS 1.9.4 04/23/2018
Call Trace:
  dump_stack+0xc9/0x11a
  ubsan_epilogue+0x9/0x45
  __ubsan_handle_shift_out_of_bounds.cold+0x61/0x10e
  uvc_get_le_value.cold+0x58/0x9f [uvcvideo]
  __uvc_query_v4l2_ctrl+0x375/0x570 [uvcvideo]
  uvc_query_v4l2_ctrl+0x9d/0xe0 [uvcvideo]
  uvc_ioctl_queryctrl+0x2d/0x40 [uvcvideo]
  v4l_queryctrl+0xad/0xe0 [videodev]
  __video_do_ioctl+0x6ff/0x870 [videodev]
  video_usercopy+0x1eb/0xa20 [videodev]
  video_ioctl2+0x15/0x20 [videodev]
  v4l2_ioctl+0x111/0x150 [videodev]
  ksys_ioctl+0x9f/0xe0
  __x64_sys_ioctl+0x43/0x50
  do_syscall_64+0x60/0xf0
  entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x7f855682d427
Code: 00 00 90 48 8b 05 69 7a 0c 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 39 7a 0c 00 f7 d8 64 89 01 48
RSP: 002b:00007f8552854208 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 000027776f49d460 RCX: 00007f855682d427
RDX: 00007f85528542d0 RSI: ffffffffc0445624 RDI: 0000000000000017
RBP: 00007f8552854480 R08: 0000000000000003 R09: 00000000fffffffd
R10: 0000000000000004 R11: 0000000000000246 R12: 000027776f53ad58
R13: 00007f85528542d0 R14: 000027776f53ac00 R15: 00005632ec54bde0
================================================================================

Re: UBSAN: shift-out-of-bounds in drivers/media/usb/uvc/uvc_ctrl.c:781

[Fabio Estevam]

Hi Bart/Laurent,

On Tue, Aug 18, 2020 at 2:41 PM Bart Van Assche <bvanassche@acm.org> wrote:
>
> Hi,
>
> This morning I installed a debug build of kernel v5.8.1 on my laptop.
> The complaint shown below appeared in the kernel log. Is this a known
> issue?
>
> Thanks,
>
> Bart.
>
> ================================================================================
> UBSAN: shift-out-of-bounds in drivers/media/usb/uvc/uvc_ctrl.c:781:13
> shift exponent -7 is negative

Should we fix it like this?

--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -778,7 +778,7 @@ static s32 uvc_get_le_value(struct
uvc_control_mapping *mapping,
                value |= offset > 0 ? (byte >> offset) : (byte << (-offset));
                bits -= 8 - (offset > 0 ? offset : 0);
                offset -= 8;
-               mask = (1 << bits) - 1;
+               mask = (1LL << bits) - 1;
        }

Re: UBSAN: shift-out-of-bounds in drivers/media/usb/uvc/uvc_ctrl.c:781

[Laurent Pinchart]

Hello,

On Tue, Aug 18, 2020 at 02:58:22PM -0300, Fabio Estevam wrote:
> Hi Bart/Laurent,
> 
> On Tue, Aug 18, 2020 at 2:41 PM Bart Van Assche <bvanassche@acm.org> wrote:
> >
> > Hi,
> >
> > This morning I installed a debug build of kernel v5.8.1 on my laptop.
> > The complaint shown below appeared in the kernel log. Is this a known
> > issue?
> >
> > ================================================================================
> > UBSAN: shift-out-of-bounds in drivers/media/usb/uvc/uvc_ctrl.c:781:13
> > shift exponent -7 is negative
> 
> Should we fix it like this?
> 
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -778,7 +778,7 @@ static s32 uvc_get_le_value(struct
> uvc_control_mapping *mapping,
>                 value |= offset > 0 ? (byte >> offset) : (byte << (-offset));
>                 bits -= 8 - (offset > 0 ? offset : 0);
>                 offset -= 8;
> -               mask = (1 << bits) - 1;
> +               mask = (1LL << bits) - 1;
>         }

No, the issue is that bits is equal to -7, 1LL won't change that.

Once bits become negative, the loop stops, and the mask value isn't used
afterwards. This would only cause an issue if a shift with a negative
value generated side effects (such as a trap for instance) on top of
producing an incorrect result. Can this happen ? I suppose we should
silence the warning even if it's a false positive, as it doesn't look
good in the kernel log.

-- 
Regards,

Laurent Pinchart

[PATCH] media: uvc: Silence shift-out-of-bounds warning

[Laurent Pinchart]

UBSAN reports a shift-out-of-bounds warning in uvc_get_le_value(). The
report is correct, but the issue should be harmless as the computed
value isn't used when the shift is negative. This may however cause
incorrect behaviour if a negative shift could generate adverse side
effects (such as a trap on some architectures for instance).

Regardless of whether that may happen or not, silence the warning as a
full WARN backtrace isn't nice.

Reported-by: Bart Van Assche <bvanassche@acm.org>
Fixes: c0efd232929c ("V4L/DVB (8145a): USB Video Class driver")
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 6c37aa018ad5..b2cdee0f7763 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -773,12 +773,16 @@ static s32 uvc_get_le_value(struct uvc_control_mapping *mapping,
 	offset &= 7;
 	mask = ((1LL << bits) - 1) << offset;
 
-	for (; bits > 0; data++) {
+	while (1) {
 		u8 byte = *data & mask;
 		value |= offset > 0 ? (byte >> offset) : (byte << (-offset));
 		bits -= 8 - (offset > 0 ? offset : 0);
+		if (bits <= 0)
+			break;
+
 		offset -= 8;
 		mask = (1 << bits) - 1;
+		data++;
 	}
 
 	/* Sign-extend the value if needed. */
-- 
Regards,

Laurent Pinchart

Re: UBSAN: shift-out-of-bounds in drivers/media/usb/uvc/uvc_ctrl.c:781

[Fabio Estevam]

Hi Laurent,

On Tue, Aug 18, 2020 at 8:54 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:

> No, the issue is that bits is equal to -7, 1LL won't change that.
>
> Once bits become negative, the loop stops, and the mask value isn't used
> afterwards. This would only cause an issue if a shift with a negative
> value generated side effects (such as a trap for instance) on top of
> producing an incorrect result. Can this happen ? I suppose we should
> silence the warning even if it's a false positive, as it doesn't look
> good in the kernel log.

Yes, you are right.

One way to avoid the negative shift:

--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -778,7 +778,8 @@ static s32 uvc_get_le_value(struct
uvc_control_mapping *mapping,
                value |= offset > 0 ? (byte >> offset) : (byte << (-offset));
                bits -= 8 - (offset > 0 ? offset : 0);
                offset -= 8;
-               mask = (1 << bits) - 1;
+               if (bits > 0)
+                       mask = (1 << bits) - 1;
        }

        /* Sign-extend the value if needed. */

Just saw your patch where you quit the while(1) if bits <=0. Thanks!

Re: [PATCH] media: uvc: Silence shift-out-of-bounds warning

[Bart Van Assche]

On 2020-08-18 17:03, Laurent Pinchart wrote:
> UBSAN reports a shift-out-of-bounds warning in uvc_get_le_value(). The
> report is correct, but the issue should be harmless as the computed
> value isn't used when the shift is negative. This may however cause
> incorrect behaviour if a negative shift could generate adverse side
> effects (such as a trap on some architectures for instance).
> 
> Regardless of whether that may happen or not, silence the warning as a
> full WARN backtrace isn't nice.
> 
> Reported-by: Bart Van Assche <bvanassche@acm.org>
> Fixes: c0efd232929c ("V4L/DVB (8145a): USB Video Class driver")
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 6c37aa018ad5..b2cdee0f7763 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -773,12 +773,16 @@ static s32 uvc_get_le_value(struct uvc_control_mapping *mapping,
>  	offset &= 7;
>  	mask = ((1LL << bits) - 1) << offset;
>  
> -	for (; bits > 0; data++) {
> +	while (1) {
>  		u8 byte = *data & mask;
>  		value |= offset > 0 ? (byte >> offset) : (byte << (-offset));
>  		bits -= 8 - (offset > 0 ? offset : 0);
> +		if (bits <= 0)
> +			break;
> +
>  		offset -= 8;
>  		mask = (1 << bits) - 1;
> +		data++;
>  	}
>  
>  	/* Sign-extend the value if needed. */

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Thanks for having addressed this quickly!

Bart.

Re: [PATCH] media: uvc: Silence shift-out-of-bounds warning

[Laurent Pinchart]

Hi Bart,

On Wed, Aug 19, 2020 at 08:25:27PM -0700, Bart Van Assche wrote:
> On 2020-08-18 17:03, Laurent Pinchart wrote:
> > UBSAN reports a shift-out-of-bounds warning in uvc_get_le_value(). The
> > report is correct, but the issue should be harmless as the computed
> > value isn't used when the shift is negative. This may however cause
> > incorrect behaviour if a negative shift could generate adverse side
> > effects (such as a trap on some architectures for instance).
> > 
> > Regardless of whether that may happen or not, silence the warning as a
> > full WARN backtrace isn't nice.
> > 
> > Reported-by: Bart Van Assche <bvanassche@acm.org>
> > Fixes: c0efd232929c ("V4L/DVB (8145a): USB Video Class driver")
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  drivers/media/usb/uvc/uvc_ctrl.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index 6c37aa018ad5..b2cdee0f7763 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -773,12 +773,16 @@ static s32 uvc_get_le_value(struct uvc_control_mapping *mapping,
> >  	offset &= 7;
> >  	mask = ((1LL << bits) - 1) << offset;
> >  
> > -	for (; bits > 0; data++) {
> > +	while (1) {
> >  		u8 byte = *data & mask;
> >  		value |= offset > 0 ? (byte >> offset) : (byte << (-offset));
> >  		bits -= 8 - (offset > 0 ? offset : 0);
> > +		if (bits <= 0)
> > +			break;
> > +
> >  		offset -= 8;
> >  		mask = (1 << bits) - 1;
> > +		data++;
> >  	}
> >  
> >  	/* Sign-extend the value if needed. */
> 
> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
> 
> Thanks for having addressed this quickly!

You're welcome. Would you be able to test the patch to ensure it fixes
the issue on your system (and that there are no observable side effects)
?

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: uvc: Silence shift-out-of-bounds warning

[Bart Van Assche]

On 8/20/20 3:37 AM, Laurent Pinchart wrote:
> Hi Bart,
> 
> On Wed, Aug 19, 2020 at 08:25:27PM -0700, Bart Van Assche wrote:
>> On 2020-08-18 17:03, Laurent Pinchart wrote:
>>> UBSAN reports a shift-out-of-bounds warning in uvc_get_le_value(). The
>>> report is correct, but the issue should be harmless as the computed
>>> value isn't used when the shift is negative. This may however cause
>>> incorrect behaviour if a negative shift could generate adverse side
>>> effects (such as a trap on some architectures for instance).
>>>
>>> Regardless of whether that may happen or not, silence the warning as a
>>> full WARN backtrace isn't nice.
>>>
>>> Reported-by: Bart Van Assche <bvanassche@acm.org>
>>> Fixes: c0efd232929c ("V4L/DVB (8145a): USB Video Class driver")
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> ---
>>>   drivers/media/usb/uvc/uvc_ctrl.c | 6 +++++-
>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
>>> index 6c37aa018ad5..b2cdee0f7763 100644
>>> --- a/drivers/media/usb/uvc/uvc_ctrl.c
>>> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
>>> @@ -773,12 +773,16 @@ static s32 uvc_get_le_value(struct uvc_control_mapping *mapping,
>>>   	offset &= 7;
>>>   	mask = ((1LL << bits) - 1) << offset;
>>>   
>>> -	for (; bits > 0; data++) {
>>> +	while (1) {
>>>   		u8 byte = *data & mask;
>>>   		value |= offset > 0 ? (byte >> offset) : (byte << (-offset));
>>>   		bits -= 8 - (offset > 0 ? offset : 0);
>>> +		if (bits <= 0)
>>> +			break;
>>> +
>>>   		offset -= 8;
>>>   		mask = (1 << bits) - 1;
>>> +		data++;
>>>   	}
>>>   
>>>   	/* Sign-extend the value if needed. */
>>
>> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
>>
>> Thanks for having addressed this quickly!
> 
> You're welcome. Would you be able to test the patch to ensure it fixes
> the issue on your system (and that there are no observable side effects)
> ?

This patch works fine on top of kernel v5.8.2 with UBSAN enabled (videocalls
still work fine), hence:

Tested-by: Bart Van Assche <bvanassche@acm.org>