| Message ID | 20210520202056.GB1216852@rowland.harvard.edu (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | USB: core: WARN if pipe direction != setup packet direction | expand |
On Thu, May 20, 2021 at 04:20:56PM -0400, Alan Stern wrote: > When a control URB is submitted, the direction indicated by URB's pipe > member is supposed to match the direction indicated by the setup > packet's bRequestType member. A mismatch could lead to trouble, > depending on which field the host controller drivers use for > determining the actual direction. > > This shouldn't ever happen; it would represent a careless bug in a > kernel driver somewhere. This patch adds a dev_WARN to let people > know about the potential problem. > > Suggested-by: "Geoffrey D. Bennett" <g@b4.vu> > Signed-off-by: Alan Stern <stern@rowland.harvard.edu> > > --- > > > [as1960] > > > drivers/usb/core/urb.c | 3 +++ > 1 file changed, 3 insertions(+) > > Index: usb-devel/drivers/usb/core/urb.c > =================================================================== > --- usb-devel.orig/drivers/usb/core/urb.c > +++ usb-devel/drivers/usb/core/urb.c > @@ -407,6 +407,9 @@ int usb_submit_urb(struct urb *urb, gfp_ > return -ENOEXEC; > is_out = !(setup->bRequestType & USB_DIR_IN) || > !setup->wLength; > + if (usb_pipeout(urb->pipe) != is_out) > + dev_WARN(&dev->dev, "BOGUS control dir, pipe %x doesn't match bRequestType %x\n", > + urb->pipe, setup->bRequestType); > } else { > is_out = usb_endpoint_dir_out(&ep->desc); > } While I agree with intention here, I'm worried that this will start flooding the logs of users. So first, this should probably be rate limited. Second, did you try to estimate how many call sites that get this wrong? I always felt a bit pedantic when pointing out that the pipe direction should match the request type to driver author's during review when (in almost all cases?) this hasn't really mattered. I fear we may have accumulated a fairly large number of these mismatches over the years but I haven't verified that. Johan
On Fri, May 21, 2021 at 10:03:26AM +0200, Johan Hovold wrote: > On Thu, May 20, 2021 at 04:20:56PM -0400, Alan Stern wrote: > > When a control URB is submitted, the direction indicated by URB's pipe > > member is supposed to match the direction indicated by the setup > > packet's bRequestType member. A mismatch could lead to trouble, > > depending on which field the host controller drivers use for > > determining the actual direction. > > > > This shouldn't ever happen; it would represent a careless bug in a > > kernel driver somewhere. This patch adds a dev_WARN to let people > > know about the potential problem. > > > > Suggested-by: "Geoffrey D. Bennett" <g@b4.vu> > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu> > > > > --- > > > > > > [as1960] > > > > > > drivers/usb/core/urb.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > Index: usb-devel/drivers/usb/core/urb.c > > =================================================================== > > --- usb-devel.orig/drivers/usb/core/urb.c > > +++ usb-devel/drivers/usb/core/urb.c > > @@ -407,6 +407,9 @@ int usb_submit_urb(struct urb *urb, gfp_ > > return -ENOEXEC; > > is_out = !(setup->bRequestType & USB_DIR_IN) || > > !setup->wLength; > > + if (usb_pipeout(urb->pipe) != is_out) > > + dev_WARN(&dev->dev, "BOGUS control dir, pipe %x doesn't match bRequestType %x\n", > > + urb->pipe, setup->bRequestType); > > } else { > > is_out = usb_endpoint_dir_out(&ep->desc); > > } > > While I agree with intention here, I'm worried that this will start > flooding the logs of users. > > So first, this should probably be rate limited. That's a good idea, this could get very noisy very quickly if one driver got it wrong. thanks, greg k-h
On Fri, May 21, 2021 at 10:03:26AM +0200, Johan Hovold wrote: > On Thu, May 20, 2021 at 04:20:56PM -0400, Alan Stern wrote: > > When a control URB is submitted, the direction indicated by URB's pipe > > member is supposed to match the direction indicated by the setup > > packet's bRequestType member. A mismatch could lead to trouble, > > depending on which field the host controller drivers use for > > determining the actual direction. > > > > This shouldn't ever happen; it would represent a careless bug in a > > kernel driver somewhere. This patch adds a dev_WARN to let people > > know about the potential problem. > > > > Suggested-by: "Geoffrey D. Bennett" <g@b4.vu> > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu> > > > > --- > > > > > > [as1960] > > > > > > drivers/usb/core/urb.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > Index: usb-devel/drivers/usb/core/urb.c > > =================================================================== > > --- usb-devel.orig/drivers/usb/core/urb.c > > +++ usb-devel/drivers/usb/core/urb.c > > @@ -407,6 +407,9 @@ int usb_submit_urb(struct urb *urb, gfp_ > > return -ENOEXEC; > > is_out = !(setup->bRequestType & USB_DIR_IN) || > > !setup->wLength; > > + if (usb_pipeout(urb->pipe) != is_out) > > + dev_WARN(&dev->dev, "BOGUS control dir, pipe %x doesn't match bRequestType %x\n", > > + urb->pipe, setup->bRequestType); > > } else { > > is_out = usb_endpoint_dir_out(&ep->desc); > > } > > While I agree with intention here, I'm worried that this will start > flooding the logs of users. > > So first, this should probably be rate limited. This could actually be done using WARN_ON_ONCE() as we don't have to worry about syzbot fuzzing descriptors here (all control endpoints are bidirectional). > Second, did you try to estimate how many call sites that get this wrong? > I always felt a bit pedantic when pointing out that the pipe direction > should match the request type to driver author's during review when (in > almost all cases?) this hasn't really mattered. I fear we may have > accumulated a fairly large number of these mismatches over the years but > I haven't verified that. I did a quick review of all ctrlpipe-macro uses in usb/misc and usb/serial and found two instances. A simple grep pattern looking for explicit USB_DIR/ctrlpipe mismatches caught another five tree wide (not including the sound/usb/mixer_scarlett_gen2.c which Geoffrey reported), but there are likely more of those out there as the request type is often not that explicit. I've prepared patches for the above, excluding the sound driver Geoffrey said he was fixing. Johan
On Fri, May 21, 2021 at 10:03:26AM +0200, Johan Hovold wrote: > On Thu, May 20, 2021 at 04:20:56PM -0400, Alan Stern wrote: > > + if (usb_pipeout(urb->pipe) != is_out) > > + dev_WARN(&dev->dev, "BOGUS control dir, pipe %x doesn't match bRequestType %x\n", > > + urb->pipe, setup->bRequestType); > While I agree with intention here, I'm worried that this will start > flooding the logs of users. > > So first, this should probably be rate limited. So change it to dev_WARN_ONCE()? You and Greg think that will be good enough? > Second, did you try to estimate how many call sites that get this wrong? No. I couldn't think of a good way to search for them. My guess is that the number isn't terribly big, but I don't know. > I always felt a bit pedantic when pointing out that the pipe direction > should match the request type to driver author's during review when (in > almost all cases?) this hasn't really mattered. I fear we may have > accumulated a fairly large number of these mismatches over the years but > I haven't verified that. Alan Stern
On Fri, May 21, 2021 at 03:17:46PM +0200, Johan Hovold wrote: > On Fri, May 21, 2021 at 10:03:26AM +0200, Johan Hovold wrote: > > So first, this should probably be rate limited. > > This could actually be done using WARN_ON_ONCE() as we don't have to > worry about syzbot fuzzing descriptors here (all control endpoints are > bidirectional). > > > Second, did you try to estimate how many call sites that get this wrong? > > I always felt a bit pedantic when pointing out that the pipe direction > > should match the request type to driver author's during review when (in > > almost all cases?) this hasn't really mattered. I fear we may have > > accumulated a fairly large number of these mismatches over the years but > > I haven't verified that. > > I did a quick review of all ctrlpipe-macro uses in usb/misc and > usb/serial and found two instances. > > A simple grep pattern looking for explicit USB_DIR/ctrlpipe mismatches > caught another five tree wide (not including the > sound/usb/mixer_scarlett_gen2.c which Geoffrey reported), but there are > likely more of those out there as the request type is often not that > explicit. > > I've prepared patches for the above, excluding the sound driver Geoffrey > said he was fixing. Many thanks for making the effort to do this. Alan Stern
On Fri, May 21, 2021 at 10:38:23AM -0400, Alan Stern wrote: > On Fri, May 21, 2021 at 10:03:26AM +0200, Johan Hovold wrote: > > On Thu, May 20, 2021 at 04:20:56PM -0400, Alan Stern wrote: > > > > + if (usb_pipeout(urb->pipe) != is_out) > > > + dev_WARN(&dev->dev, "BOGUS control dir, pipe %x doesn't match bRequestType %x\n", > > > + urb->pipe, setup->bRequestType); > > > While I agree with intention here, I'm worried that this will start > > flooding the logs of users. > > > > So first, this should probably be rate limited. > > So change it to dev_WARN_ONCE()? You and Greg think that will be good > enough? I think so, yes. In the unlikely event that there are more than one driver in use that gets this wrong in a system it'll only take a little longer to root them all out. Johan
Index: usb-devel/drivers/usb/core/urb.c =================================================================== --- usb-devel.orig/drivers/usb/core/urb.c +++ usb-devel/drivers/usb/core/urb.c @@ -407,6 +407,9 @@ int usb_submit_urb(struct urb *urb, gfp_ return -ENOEXEC; is_out = !(setup->bRequestType & USB_DIR_IN) || !setup->wLength; + if (usb_pipeout(urb->pipe) != is_out) + dev_WARN(&dev->dev, "BOGUS control dir, pipe %x doesn't match bRequestType %x\n", + urb->pipe, setup->bRequestType); } else { is_out = usb_endpoint_dir_out(&ep->desc); }
When a control URB is submitted, the direction indicated by URB's pipe member is supposed to match the direction indicated by the setup packet's bRequestType member. A mismatch could lead to trouble, depending on which field the host controller drivers use for determining the actual direction. This shouldn't ever happen; it would represent a careless bug in a kernel driver somewhere. This patch adds a dev_WARN to let people know about the potential problem. Suggested-by: "Geoffrey D. Bennett" <g@b4.vu> Signed-off-by: Alan Stern <stern@rowland.harvard.edu> --- [as1960] drivers/usb/core/urb.c | 3 +++ 1 file changed, 3 insertions(+)