All of lore.kernel.org
 help / color / mirror / Atom feed
* USB: change dev_WARN to dev_err triggerable from user space
@ 2018-09-04 11:15 Johan Hovold
  0 siblings, 0 replies; 6+ messages in thread
From: Johan Hovold @ 2018-09-04 11:15 UTC (permalink / raw)
  To: Oliver Neukum; +Cc: Johan Hovold, gregkh, linux-usb, Alan Stern

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* USB: change dev_WARN to dev_err triggerable from user space
@ 2018-09-05  7:40 Oliver Neukum
  0 siblings, 0 replies; 6+ messages in thread
From: Oliver Neukum @ 2018-09-05  7:40 UTC (permalink / raw)
  To: Alan Stern, Johan Hovold; +Cc: gregkh, linux-usb

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* USB: change dev_WARN to dev_err triggerable from user space
@ 2018-09-04 19:18 Alan Stern
  0 siblings, 0 replies; 6+ messages in thread
From: Alan Stern @ 2018-09-04 19:18 UTC (permalink / raw)
  To: Johan Hovold; +Cc: Oliver Neukum, gregkh, linux-usb

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* USB: change dev_WARN to dev_err triggerable from user space
@ 2018-09-04 10:21 Oliver Neukum
  0 siblings, 0 replies; 6+ messages in thread
From: Oliver Neukum @ 2018-09-04 10:21 UTC (permalink / raw)
  To: Johan Hovold; +Cc: gregkh, linux-usb

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* USB: change dev_WARN to dev_err triggerable from user space
@ 2018-09-04  9:31 Johan Hovold
  0 siblings, 0 replies; 6+ messages in thread
From: Johan Hovold @ 2018-09-04  9:31 UTC (permalink / raw)
  To: Oliver Neukum; +Cc: gregkh, linux-usb

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* USB: change dev_WARN to dev_err triggerable from user space
@ 2018-09-04  8:44 Oliver Neukum
  0 siblings, 0 replies; 6+ messages in thread
From: Oliver Neukum @ 2018-09-04  8:44 UTC (permalink / raw)
  To: gregkh, linux-usb; +Cc: Oliver Neukum

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(-)

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);
 
 	/*

^ permalink raw reply related	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-09-05  7:40 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-04 11:15 USB: change dev_WARN to dev_err triggerable from user space Johan Hovold
  -- strict thread matches above, loose matches on Subject: below --
2018-09-05  7:40 Oliver Neukum
2018-09-04 19:18 Alan Stern
2018-09-04 10:21 Oliver Neukum
2018-09-04  9:31 Johan Hovold
2018-09-04  8:44 Oliver Neukum

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.