USB Denial Of Service

USB Denial Of Service

[Alan Stern]

Greg, Oliver, or anyone else:

Questions:

If a broken or malicious device causes a USB class driver to add a 
thousand (or more) error messages per second to the kernel log, 
indefinitely, would that be considered a form of DOS?

Should the driver be fixed?

What is an acceptable rate for an unending stream of error messages?  
Once a second?  Once a minute?

At what point should the driver give up and stop trying to communicate 
with the device?

(These are not moot questions.  There are indeed drivers, and probably 
not just in the USB subsystem, subject to this sort of behavior.)

Alan Stern

Re: USB Denial Of Service

[Alan Stern]

On Tue, Jun 11, 2024 at 10:35:12AM -0400, Alan Stern wrote:
> Greg, Oliver, or anyone else:
> 
> Questions:
> 
> If a broken or malicious device causes a USB class driver to add a 
> thousand (or more) error messages per second to the kernel log, 
> indefinitely, would that be considered a form of DOS?
> 
> Should the driver be fixed?
> 
> What is an acceptable rate for an unending stream of error messages?  
> Once a second?  Once a minute?
> 
> At what point should the driver give up and stop trying to communicate 
> with the device?
> 
> (These are not moot questions.  There are indeed drivers, and probably 
> not just in the USB subsystem, subject to this sort of behavior.)

Along those lines, what do you think of the following patch for handling 
-EPROTO, -EILSEQ, or -ETIME status values for the interrupt URB in the 
cdc-wdm driver?  After one of those errors, the URB is immediately 
resubmitted, so the error is likely to occur again no more than a 
millisecond later.  Changing dev_err() to dev_dbg() prevents log 
spamming.

Alternatively, the driver could avoid resubmitting the URB when one of 
those errors occurs.  This is perhaps less appropriate, because these 
kinds of errors can be transient (although that is normally rare).

Alan Stern



Index: usb-devel/drivers/usb/class/cdc-wdm.c
===================================================================
--- usb-devel.orig/drivers/usb/class/cdc-wdm.c
+++ usb-devel/drivers/usb/class/cdc-wdm.c
@@ -266,14 +266,14 @@ static void wdm_int_callback(struct urb
 			dev_err(&desc->intf->dev, "Stall on int endpoint\n");
 			goto sw; /* halt is cleared in work */
 		default:
-			dev_err(&desc->intf->dev,
+			dev_dbg(&desc->intf->dev,
 				"nonzero urb status received: %d\n", status);
 			break;
 		}
 	}
 
 	if (urb->actual_length < sizeof(struct usb_cdc_notification)) {
-		dev_err(&desc->intf->dev, "wdm_int_callback - %d bytes\n",
+		dev_dbg(&desc->intf->dev, "wdm_int_callback - %d bytes\n",
 			urb->actual_length);
 		goto exit;
 	}

Re: USB Denial Of Service

[Oliver Neukum]

On 11.06.24 21:09, Alan Stern wrote:

> Alternatively, the driver could avoid resubmitting the URB when one of
> those errors occurs.  This is perhaps less appropriate, because these
> kinds of errors can be transient (although that is normally rare).

Optimally we'd wait a small delay and resubmit after that for a limited
number of attempts. In fact optimally we'd add helpers to do so for
all drivers to use.

However, to make this wor with suspend, reset and disconnect would
take work. Is a corner case worth that?

> Index: usb-devel/drivers/usb/class/cdc-wdm.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/class/cdc-wdm.c
> +++ usb-devel/drivers/usb/class/cdc-wdm.c
> @@ -266,14 +266,14 @@ static void wdm_int_callback(struct urb
>   			dev_err(&desc->intf->dev, "Stall on int endpoint\n");
>   			goto sw; /* halt is cleared in work */
>   		default:
> -			dev_err(&desc->intf->dev,
> +			dev_dbg(&desc->intf->dev,

Good idea.

>   				"nonzero urb status received: %d\n", status);
>   			break;
>   		}
>   	}
>   
>   	if (urb->actual_length < sizeof(struct usb_cdc_notification)) {
> -		dev_err(&desc->intf->dev, "wdm_int_callback - %d bytes\n",
> +		dev_dbg(&desc->intf->dev, "wdm_int_callback - %d bytes\n",

Not so good idea. If that happens it points to either a bug in the firmware
or a malicious device. Such things need to be logged.

	Regards
		Oliver

Re: USB Denial Of Service

[Greg KH]

On Tue, Jun 11, 2024 at 03:09:32PM -0400, Alan Stern wrote:
> On Tue, Jun 11, 2024 at 10:35:12AM -0400, Alan Stern wrote:
> > Greg, Oliver, or anyone else:
> > 
> > Questions:
> > 
> > If a broken or malicious device causes a USB class driver to add a 
> > thousand (or more) error messages per second to the kernel log, 
> > indefinitely, would that be considered a form of DOS?
> > 
> > Should the driver be fixed?
> > 
> > What is an acceptable rate for an unending stream of error messages?  
> > Once a second?  Once a minute?
> > 
> > At what point should the driver give up and stop trying to communicate 
> > with the device?
> > 
> > (These are not moot questions.  There are indeed drivers, and probably 
> > not just in the USB subsystem, subject to this sort of behavior.)
> 
> Along those lines, what do you think of the following patch for handling 
> -EPROTO, -EILSEQ, or -ETIME status values for the interrupt URB in the 
> cdc-wdm driver?  After one of those errors, the URB is immediately 
> resubmitted, so the error is likely to occur again no more than a 
> millisecond later.  Changing dev_err() to dev_dbg() prevents log 
> spamming.
> 
> Alternatively, the driver could avoid resubmitting the URB when one of 
> those errors occurs.  This is perhaps less appropriate, because these 
> kinds of errors can be transient (although that is normally rare).
> 
> Alan Stern
> 
> 
> 
> Index: usb-devel/drivers/usb/class/cdc-wdm.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/class/cdc-wdm.c
> +++ usb-devel/drivers/usb/class/cdc-wdm.c
> @@ -266,14 +266,14 @@ static void wdm_int_callback(struct urb
>  			dev_err(&desc->intf->dev, "Stall on int endpoint\n");
>  			goto sw; /* halt is cleared in work */
>  		default:
> -			dev_err(&desc->intf->dev,
> +			dev_dbg(&desc->intf->dev,
>  				"nonzero urb status received: %d\n", status);

dev_err_ratelimited() maybe instead?


>  			break;
>  		}
>  	}
>  
>  	if (urb->actual_length < sizeof(struct usb_cdc_notification)) {
> -		dev_err(&desc->intf->dev, "wdm_int_callback - %d bytes\n",
> +		dev_dbg(&desc->intf->dev, "wdm_int_callback - %d bytes\n",

Same here?

thanks,

greg k-h

Re: USB Denial Of Service

[Oliver Neukum]

On 11.06.24 16:35, Alan Stern wrote:
> Greg, Oliver, or anyone else:
> 
> Questions:
> 
> If a broken or malicious device causes a USB class driver to add a
> thousand (or more) error messages per second to the kernel log,
> indefinitely, would that be considered a form of DOS?

Yes.

> Should the driver be fixed?

If a broken device can do that, definitely.

> What is an acceptable rate for an unending stream of error messages?
> Once a second?  Once a minute?

Definitely not once a second. I'd be tempted to call a neverending stream
an issue by itself. The approach the SCSI layer takes by giving up on
a device if all else fails seems wise to me.
  
> At what point should the driver give up and stop trying to communicate
> with the device?

I would propose after five cycles of all error handling.

The exact number, as long as it is greater than 1 and a small integer
does not really matter, as long as it exists.

	Regards
		Oliver

Re: USB Denial Of Service

[Greg KH]

On Tue, Jun 11, 2024 at 10:35:12AM -0400, Alan Stern wrote:
> Greg, Oliver, or anyone else:
> 
> Questions:
> 
> If a broken or malicious device causes a USB class driver to add a 
> thousand (or more) error messages per second to the kernel log, 
> indefinitely, would that be considered a form of DOS?
> 
> Should the driver be fixed?

Good question.  Right now, by default, we "trust" usb devices to an
extent.  We have been "pushing back" that boundry over time, such that
now we will validate USB descriptors to verify that they actually are
sane before allowing a driver to bind to them, and if there's any bugs
with that, we will fix them.

But we totally trust the data stream from devices, and trust that once
an urb is submitted, they work properly.

If we wish to change that threat model, great, but that will require
those that wish to change that model to DO THE ACTUAL WORK!

I don't want to see fuzzers start to fuzz the data streams of USB
drivers and expect us to fix the bugs.  That's flat out not ok, as this
is something that right now, we do not care about.  If companies do care
about this, they need to do the work as that is NOT how Linux is
currently designed and implemented.

Same goes for other device types.  I get this conversation all the time
(had it last week at a very very very large Linux company.)  It usually
goes something like:

	Them:	We want to claim that we can trust drivers to work
		properly for malicious devices
	Me:	Wonderful, send the patches to do so, fixing up all
		subsystems that rely on them!
	Them:	No, that's something that Linux should already support.
	Me:	Why do you care about this?
	Them:	Because we want to host systems in untrusted situations.
	Me:	So you want to save money by not using a single physical
		host.
	Them:	Yes.
	Me:	Then spend some of that money to do the work to make
		this happen, do not force the community to do it for you.
	Them:	...

> What is an acceptable rate for an unending stream of error messages?  
> Once a second?  Once a minute?

The *_ratelimited() functions should handle this if you want to use
them.

> At what point should the driver give up and stop trying to communicate 
> with the device?

That's tricky, we don't have good answers for that as everyone has a
different idea of how long "flaky" devices should be able to flake out
before coming back.

> (These are not moot questions.  There are indeed drivers, and probably 
> not just in the USB subsystem, subject to this sort of behavior.)

Totally agreed.  But again, the design of Linux right now is that we
implicitly trust the hardware we are running on.  If that design
decision wants to be changed, some people need to do a ton of work to
change it.

Thanks,

greg k-h