[PATCH] usb: serial: sierra: deal with insufficiently long notifications

[PATCH] usb: serial: sierra: deal with insufficiently long notifications

[Oliver Neukum]

A malicious device can send too short messages which
would cause the driver to overflow the buffer and hit
stale data in their interpretation.
Ignore too short packets.

Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/usb/serial/sierra.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/serial/sierra.c b/drivers/usb/serial/sierra.c
index 6e443aacae07..f50833055360 100644
--- a/drivers/usb/serial/sierra.c
+++ b/drivers/usb/serial/sierra.c
@@ -575,10 +575,19 @@ static void sierra_instat_callback(struct urb *urb)
 				__func__);
 			return;
 		}
+
+		if (urb->actual_length < sizeof(struct usb_ctrlrequest))
+			goto skip_too_short;
+
 		if ((req_pkt->bRequestType == 0xA1) &&
 				(req_pkt->bRequest == 0x20)) {
 			int old_dcd_state;
-			unsigned char signals = *((unsigned char *)
+			unsigned char signals;
+
+			if (urb->actual_length < sizeof(struct usb_ctrlrequest) + 1)
+				goto skip_too_short;
+
+			signals = *((unsigned char *)
 					urb->transfer_buffer +
 					sizeof(struct usb_ctrlrequest));
 
@@ -603,6 +612,7 @@ static void sierra_instat_callback(struct urb *urb)
 
 	/* Resubmit urb so we continue receiving IRQ data */
 	if (status != -ESHUTDOWN && status != -ENOENT) {
+skip_too_short:
 		usb_mark_last_busy(serial->dev);
 		err = usb_submit_urb(urb, GFP_ATOMIC);
 		if (err && err != -EPERM)
-- 
2.55.0

Re: [PATCH] usb: serial: sierra: deal with insufficiently long notifications

[Johan Hovold]

On Tue, Jul 14, 2026 at 08:33:05PM +0200, Oliver Neukum wrote:
> A malicious device can send too short messages which
> would cause the driver to overflow the buffer and hit

"overflow" makes it sounds like memory corruption but this is just a
potential 1-byte read beyond the buffer (or of stale data inside the
buffer).

> stale data in their interpretation.
> Ignore too short packets.

Even if this was found by an LLM it was still reported to use by a
person so please add a Reported-by and Link tag along with a Fixes tag.

> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> ---
>  drivers/usb/serial/sierra.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/serial/sierra.c b/drivers/usb/serial/sierra.c
> index 6e443aacae07..f50833055360 100644
> --- a/drivers/usb/serial/sierra.c
> +++ b/drivers/usb/serial/sierra.c
> @@ -575,10 +575,19 @@ static void sierra_instat_callback(struct urb *urb)
>  				__func__);
>  			return;
>  		}
> +
> +		if (urb->actual_length < sizeof(struct usb_ctrlrequest))
> +			goto skip_too_short;
> +
>  		if ((req_pkt->bRequestType == 0xA1) &&
>  				(req_pkt->bRequest == 0x20)) {
>  			int old_dcd_state;
> -			unsigned char signals = *((unsigned char *)
> +			unsigned char signals;
> +
> +			if (urb->actual_length < sizeof(struct usb_ctrlrequest) + 1)
> +				goto skip_too_short;
> +
> +			signals = *((unsigned char *)
>  					urb->transfer_buffer +
>  					sizeof(struct usb_ctrlrequest));
>  
> @@ -603,6 +612,7 @@ static void sierra_instat_callback(struct urb *urb)
>  
>  	/* Resubmit urb so we continue receiving IRQ data */
>  	if (status != -ESHUTDOWN && status != -ENOENT) {
> +skip_too_short:

And please move the label before the conditional and rename it after
what it does (e.g. "out_resubmit" or just "resubmit").

>  		usb_mark_last_busy(serial->dev);
>  		err = usb_submit_urb(urb, GFP_ATOMIC);
>  		if (err && err != -EPERM)

Johan

Re: [PATCH] usb: serial: sierra: deal with insufficiently long notifications

[Oliver Neukum]

On 15.07.26 09:22, Johan Hovold wrote:
  
>>   	/* Resubmit urb so we continue receiving IRQ data */
>>   	if (status != -ESHUTDOWN && status != -ENOENT) {
>> +skip_too_short:
> 
> And please move the label before the conditional and rename it after

That really makes no sense. If we encounter a short packet
the status must be good. We wouldn't be allowed to look
at the packet in the first place if that weren't the case.

	Regards
		Oliver

Re: [PATCH] usb: serial: sierra: deal with insufficiently long notifications

[Johan Hovold]

On Wed, Jul 15, 2026 at 12:05:10PM +0200, Oliver Neukum wrote:
> 
> 
> On 15.07.26 09:22, Johan Hovold wrote:
>   
> >>   	/* Resubmit urb so we continue receiving IRQ data */
> >>   	if (status != -ESHUTDOWN && status != -ENOENT) {
> >> +skip_too_short:
> > 
> > And please move the label before the conditional and rename it after
> 
> That really makes no sense. If we encounter a short packet
> the status must be good. We wouldn't be allowed to look
> at the packet in the first place if that weren't the case.

It makes the code cleaner by not adding labels inside a conditional.

And we don't care about that extra status check in the theoretical case
that a malicious device ever sends a short message.

Johan