| Message ID | 20180904084441.3970-1-oneukum@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | USB: change dev_WARN to dev_err triggerable from user space | expand |
On Tue, Sep 04, 2018 at 10:44:41AM +0200, Oliver Neukum wrote: > For those people who run with panic_on_warn a WARN() triggered > from user space is a DOS. It is worth returning to dev_err() I think this should be dev_warn() unless you want to bring back the returning of errors on these conditions as well (i.e. as was the case prior to 0cb54a3e47cb ("USB: debugging code shouldn't alter control flow")). > Signed-off-by: Oliver Neukum <oneukum@suse.com> > Fixes: 0cb54a3e47cb4baf0bc7463f0a64cfeae5e35697 This should be: Fixes: 0cb54a3e47cb ("USB: debugging code shouldn't alter control flow") > Reported-by: syzbot+843efa30c8821bd69f53@syzkaller.appspotmail.com > --- > drivers/usb/core/urb.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c > index f51750bcd152..3fe65a774e6c 100644 > --- a/drivers/usb/core/urb.c > +++ b/drivers/usb/core/urb.c > @@ -475,7 +475,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags) > > /* Check that the pipe's type matches the endpoint's type */ > if (usb_urb_ep_type_check(urb)) > - dev_WARN(&dev->dev, "BOGUS urb xfer, pipe %x != type %x\n", > + dev_err(&dev->dev, "BOGUS urb xfer, pipe %x != type %x\n", > usb_pipetype(urb->pipe), pipetypes[xfertype]); > > /* Check against a simple/standard policy */ > @@ -499,7 +499,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags) > > /* warn if submitter gave bogus flags */ > if (allowed != urb->transfer_flags) > - dev_WARN(&dev->dev, "BOGUS urb flags, %x --> %x\n", > + dev_err(&dev->dev, "BOGUS urb flags, %x --> %x\n", > urb->transfer_flags, allowed); > > /* Johan
On Di, 2018-09-04 at 11:31 +0200, Johan Hovold wrote: > On Tue, Sep 04, 2018 at 10:44:41AM +0200, Oliver Neukum wrote: > > For those people who run with panic_on_warn a WARN() triggered > > from user space is a DOS. It is worth returning to dev_err() > > I think this should be dev_warn() unless you want to bring back the > returning of errors on these conditions as well (i.e. as was the case > prior to 0cb54a3e47cb ("USB: debugging code shouldn't alter control > flow")). Should I? A warning in syslog is pretty hardcore, so I have no idea whether dev_warn() is enough. Regards Oliver
On Tue, Sep 04, 2018 at 12:21:09PM +0200, Oliver Neukum wrote: > On Di, 2018-09-04 at 11:31 +0200, Johan Hovold wrote: > > On Tue, Sep 04, 2018 at 10:44:41AM +0200, Oliver Neukum wrote: > > > For those people who run with panic_on_warn a WARN() triggered > > > from user space is a DOS. It is worth returning to dev_err() > > > > I think this should be dev_warn() unless you want to bring back the > > returning of errors on these conditions as well (i.e. as was the case > > prior to 0cb54a3e47cb ("USB: debugging code shouldn't alter control > > flow")). > > Should I? A warning in syslog is pretty hardcore, so I have no idea > whether dev_warn() is enough. Perhaps there are two sides to this. If something really should not be happening and needs to be addressed (i.e. it's a driver bug) that dev_WARN is warranted. If user space can be pass in bogus flags that gets propagated to USB core, perhaps those need to be sanitised sooner (in the vain of "don't trust anything coming from user space"). In general though, I believe dev_warn() is more appropriate for cases where something odd is happening, but we try to recover and proceed anyway (e.g. by sanitising the flags without bailing out as is the case here) instead of aborting. Johan
On Tue, 4 Sep 2018, Johan Hovold wrote: > On Tue, Sep 04, 2018 at 12:21:09PM +0200, Oliver Neukum wrote: > > On Di, 2018-09-04 at 11:31 +0200, Johan Hovold wrote: > > > On Tue, Sep 04, 2018 at 10:44:41AM +0200, Oliver Neukum wrote: > > > > For those people who run with panic_on_warn a WARN() triggered > > > > from user space is a DOS. It is worth returning to dev_err() > > > > > > I think this should be dev_warn() unless you want to bring back the > > > returning of errors on these conditions as well (i.e. as was the case > > > prior to 0cb54a3e47cb ("USB: debugging code shouldn't alter control > > > flow")). > > > > Should I? A warning in syslog is pretty hardcore, so I have no idea > > whether dev_warn() is enough. > > Perhaps there are two sides to this. If something really should not be > happening and needs to be addressed (i.e. it's a driver bug) that > dev_WARN is warranted. If user space can be pass in bogus flags that > gets propagated to USB core, perhaps those need to be sanitised sooner > (in the vain of "don't trust anything coming from user space"). I'd go along with this. The usbfs code should fix or reject URBs submitted from userspace with bogus flags or an incorrect pipe value. (In fact, we already sanitize the flags to some extent, but we could do more: ISO_ASAP should apply only to isochronous URBs, and ZERO_PACKET should apply only to bulk-OUT URBS.) Similar errors coming from kernel drivers should be reported as actual bugs. Alan > In general though, I believe dev_warn() is more appropriate for cases > where something odd is happening, but we try to recover and proceed > anyway (e.g. by sanitising the flags without bailing out as is the case > here) instead of aborting. > > Johan
On Di, 2018-09-04 at 15:18 -0400, Alan Stern wrote: > On Tue, 4 Sep 2018, Johan Hovold wrote: > > > On Tue, Sep 04, 2018 at 12:21:09PM +0200, Oliver Neukum wrote: > > > On Di, 2018-09-04 at 11:31 +0200, Johan Hovold wrote: > > > > On Tue, Sep 04, 2018 at 10:44:41AM +0200, Oliver Neukum wrote: > > > > > For those people who run with panic_on_warn a WARN() triggered > > > > > from user space is a DOS. It is worth returning to dev_err() > > > > > > > > I think this should be dev_warn() unless you want to bring back the > > > > returning of errors on these conditions as well (i.e. as was the case > > > > prior to 0cb54a3e47cb ("USB: debugging code shouldn't alter control > > > > flow")). > > > > > > Should I? A warning in syslog is pretty hardcore, so I have no idea > > > whether dev_warn() is enough. > > > > Perhaps there are two sides to this. If something really should not be > > happening and needs to be addressed (i.e. it's a driver bug) that > > dev_WARN is warranted. If user space can be pass in bogus flags that > > gets propagated to USB core, perhaps those need to be sanitised sooner > > (in the vain of "don't trust anything coming from user space"). > > I'd go along with this. The usbfs code should fix or reject URBs > submitted from userspace with bogus flags or an incorrect pipe value. > (In fact, we already sanitize the flags to some extent, but we could do > more: ISO_ASAP should apply only to isochronous URBs, and ZERO_PACKET > should apply only to bulk-OUT URBS.) > > Similar errors coming from kernel drivers should be reported as actual > bugs. Very well, I am making a patch to do that. Regards Oliver
diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c index f51750bcd152..3fe65a774e6c 100644 --- a/drivers/usb/core/urb.c +++ b/drivers/usb/core/urb.c @@ -475,7 +475,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags) /* Check that the pipe's type matches the endpoint's type */ if (usb_urb_ep_type_check(urb)) - dev_WARN(&dev->dev, "BOGUS urb xfer, pipe %x != type %x\n", + dev_err(&dev->dev, "BOGUS urb xfer, pipe %x != type %x\n", usb_pipetype(urb->pipe), pipetypes[xfertype]); /* Check against a simple/standard policy */ @@ -499,7 +499,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags) /* warn if submitter gave bogus flags */ if (allowed != urb->transfer_flags) - dev_WARN(&dev->dev, "BOGUS urb flags, %x --> %x\n", + dev_err(&dev->dev, "BOGUS urb flags, %x --> %x\n", urb->transfer_flags, allowed); /*
For those people who run with panic_on_warn a WARN() triggered from user space is a DOS. It is worth returning to dev_err() Signed-off-by: Oliver Neukum <oneukum@suse.com> Fixes: 0cb54a3e47cb4baf0bc7463f0a64cfeae5e35697 Reported-by: syzbot+843efa30c8821bd69f53@syzkaller.appspotmail.com --- drivers/usb/core/urb.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)