Re: USB Denial Of Service

[Greg KH <gregkh@linuxfoundation.org>]

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