| Message ID | 20180905100703.27963-2-oneukum@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] USB: usbdevfs: sanitize flags more | expand |
On Wed, Sep 05, 2018 at 12:07:03PM +0200, Oliver Neukum wrote: > If we filter flags before they reach the core we need to generate our > own warnings. > > Signed-off-by: Oliver Neukum <oneukum@suse.com> > Fixes: 0cb54a3e47cb ("USB: debugging code shouldn't alter control flow") > --- > drivers/usb/core/devio.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c > index 263dd2f309fb..244417d0dfd1 100644 > --- a/drivers/usb/core/devio.c > +++ b/drivers/usb/core/devio.c > @@ -1697,6 +1697,11 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb > u |= URB_NO_INTERRUPT; > as->urb->transfer_flags = u; > > + if (!allow_short && uurb->flags & USBDEVFS_URB_SHORT_NOT_OK) > + dev_warn(&ps->dev->dev, "Requested nonsensical USBDEVFS_URB_SHORT_NOT_OK.\n"); > + if (!allow_zero && uurb->flags & USBDEVFS_URB_ZERO_PACKET) > + dev_warn(&ps->dev->dev, "Requested nonsensical USBDEVFS_URB_ZERO_PACKET.\n"); We should not make it trivial for userspace to spam the kernel log if at all possible. Returning an error is probably the better thing to do here, not just silently fix it up or ignore it. thanks, greg k-h
On Mi, 2018-09-05 at 14:19 +0200, Greg KH wrote: > On Wed, Sep 05, 2018 at 12:07:03PM +0200, Oliver Neukum wrote: > > If we filter flags before they reach the core we need to generate our > > own warnings. > > > > Signed-off-by: Oliver Neukum <oneukum@suse.com> > > Fixes: 0cb54a3e47cb ("USB: debugging code shouldn't alter control flow") > > --- > > drivers/usb/core/devio.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c > > index 263dd2f309fb..244417d0dfd1 100644 > > --- a/drivers/usb/core/devio.c > > +++ b/drivers/usb/core/devio.c > > @@ -1697,6 +1697,11 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb > > u |= URB_NO_INTERRUPT; > > as->urb->transfer_flags = u; > > > > + if (!allow_short && uurb->flags & USBDEVFS_URB_SHORT_NOT_OK) > > + dev_warn(&ps->dev->dev, "Requested nonsensical USBDEVFS_URB_SHORT_NOT_OK.\n"); > > + if (!allow_zero && uurb->flags & USBDEVFS_URB_ZERO_PACKET) > > + dev_warn(&ps->dev->dev, "Requested nonsensical USBDEVFS_URB_ZERO_PACKET.\n"); > > We should not make it trivial for userspace to spam the kernel log if at > all possible. Returning an error is probably the better thing to do > here, not just silently fix it up or ignore it. That means a change in the API in a way that makes orking systems fail. Do you want an extra version for stable? Regards Oliver
On Wed, Sep 05, 2018 at 03:02:48PM +0200, Oliver Neukum wrote: > On Mi, 2018-09-05 at 14:19 +0200, Greg KH wrote: > > On Wed, Sep 05, 2018 at 12:07:03PM +0200, Oliver Neukum wrote: > > > If we filter flags before they reach the core we need to generate our > > > own warnings. > > > > > > Signed-off-by: Oliver Neukum <oneukum@suse.com> > > > Fixes: 0cb54a3e47cb ("USB: debugging code shouldn't alter control flow") > > > --- > > > drivers/usb/core/devio.c | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c > > > index 263dd2f309fb..244417d0dfd1 100644 > > > --- a/drivers/usb/core/devio.c > > > +++ b/drivers/usb/core/devio.c > > > @@ -1697,6 +1697,11 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb > > > u |= URB_NO_INTERRUPT; > > > as->urb->transfer_flags = u; > > > > > > + if (!allow_short && uurb->flags & USBDEVFS_URB_SHORT_NOT_OK) > > > + dev_warn(&ps->dev->dev, "Requested nonsensical USBDEVFS_URB_SHORT_NOT_OK.\n"); > > > + if (!allow_zero && uurb->flags & USBDEVFS_URB_ZERO_PACKET) > > > + dev_warn(&ps->dev->dev, "Requested nonsensical USBDEVFS_URB_ZERO_PACKET.\n"); > > > > We should not make it trivial for userspace to spam the kernel log if at > > all possible. Returning an error is probably the better thing to do > > here, not just silently fix it up or ignore it. > > That means a change in the API in a way that makes orking systems fail. Ah, good point. I guess they were hitting the same dev_WARN() messages today anyway, right? > Do you want an extra version for stable? No, but why was this patch not marked for stable? thanks, greg k-h
On Mi, 2018-09-05 at 15:07 +0200, Greg KH wrote: > On Wed, Sep 05, 2018 at 03:02:48PM +0200, Oliver Neukum wrote: > > On Mi, 2018-09-05 at 14:19 +0200, Greg KH wrote: > > > On Wed, Sep 05, 2018 at 12:07:03PM +0200, Oliver Neukum wrote: > > > > + if (!allow_short && uurb->flags & USBDEVFS_URB_SHORT_NOT_OK) > > > > + dev_warn(&ps->dev->dev, "Requested nonsensical USBDEVFS_URB_SHORT_NOT_OK.\n"); > > > > + if (!allow_zero && uurb->flags & USBDEVFS_URB_ZERO_PACKET) > > > > + dev_warn(&ps->dev->dev, "Requested nonsensical USBDEVFS_URB_ZERO_PACKET.\n"); > > > > > > We should not make it trivial for userspace to spam the kernel log if at > > > all possible. Returning an error is probably the better thing to do > > > here, not just silently fix it up or ignore it. > > > > That means a change in the API in a way that makes orking systems fail. > > Ah, good point. Well, but do we want to do this in the next major release even if we cannot do it in a stable release? > I guess they were hitting the same dev_WARN() messages > today anyway, right? Yes. And for a kernel problem you really want the stack traces. Still, that does not tell us that we want to print a message if user space messes up. So dev_warn() or nothing? > > Do you want an extra version for stable? > > No, but why was this patch not marked for stable? I was under the impression that it was. This is a separate patch because you could argue that it is unnecessary or that stable and the next release should diverge on whether to take it. Regards Oliver
On Thu, 6 Sep 2018, Oliver Neukum wrote: > On Mi, 2018-09-05 at 15:07 +0200, Greg KH wrote: > > On Wed, Sep 05, 2018 at 03:02:48PM +0200, Oliver Neukum wrote: > > > On Mi, 2018-09-05 at 14:19 +0200, Greg KH wrote: > > > > On Wed, Sep 05, 2018 at 12:07:03PM +0200, Oliver Neukum wrote: > > > > > > + if (!allow_short && uurb->flags & USBDEVFS_URB_SHORT_NOT_OK) > > > > > + dev_warn(&ps->dev->dev, "Requested nonsensical USBDEVFS_URB_SHORT_NOT_OK.\n"); > > > > > + if (!allow_zero && uurb->flags & USBDEVFS_URB_ZERO_PACKET) > > > > > + dev_warn(&ps->dev->dev, "Requested nonsensical USBDEVFS_URB_ZERO_PACKET.\n"); > > > > > > > > We should not make it trivial for userspace to spam the kernel log if at > > > > all possible. Returning an error is probably the better thing to do > > > > here, not just silently fix it up or ignore it. > > > > > > That means a change in the API in a way that makes orking systems fail. > > > > Ah, good point. > > Well, but do we want to do this in the next major release even if we > cannot do it in a stable release? > > > I guess they were hitting the same dev_WARN() messages > > today anyway, right? > > Yes. And for a kernel problem you really want the stack traces. > Still, that does not tell us that we want to print a message if > user space messes up. So dev_warn() or nothing? An alternative is for usbfs to silently fix the flags when they are wrong. Would that be any better? Alan Stern > > > Do you want an extra version for stable? > > > > No, but why was this patch not marked for stable? > > I was under the impression that it was. This is a separate > patch because you could argue that it is unnecessary or that stable > and the next release should diverge on whether to take it. > > Regards > Oliver
On Thu, Sep 06, 2018 at 11:34:04AM -0400, Alan Stern wrote: > On Thu, 6 Sep 2018, Oliver Neukum wrote: > > > On Mi, 2018-09-05 at 15:07 +0200, Greg KH wrote: > > > On Wed, Sep 05, 2018 at 03:02:48PM +0200, Oliver Neukum wrote: > > > > On Mi, 2018-09-05 at 14:19 +0200, Greg KH wrote: > > > > > On Wed, Sep 05, 2018 at 12:07:03PM +0200, Oliver Neukum wrote: > > > > > > > > + if (!allow_short && uurb->flags & USBDEVFS_URB_SHORT_NOT_OK) > > > > > > + dev_warn(&ps->dev->dev, "Requested nonsensical USBDEVFS_URB_SHORT_NOT_OK.\n"); > > > > > > + if (!allow_zero && uurb->flags & USBDEVFS_URB_ZERO_PACKET) > > > > > > + dev_warn(&ps->dev->dev, "Requested nonsensical USBDEVFS_URB_ZERO_PACKET.\n"); > > > > > > > > > > We should not make it trivial for userspace to spam the kernel log if at > > > > > all possible. Returning an error is probably the better thing to do > > > > > here, not just silently fix it up or ignore it. > > > > > > > > That means a change in the API in a way that makes orking systems fail. > > > > > > Ah, good point. > > > > Well, but do we want to do this in the next major release even if we > > cannot do it in a stable release? > > > > > I guess they were hitting the same dev_WARN() messages > > > today anyway, right? > > > > Yes. And for a kernel problem you really want the stack traces. > > Still, that does not tell us that we want to print a message if > > user space messes up. So dev_warn() or nothing? > > An alternative is for usbfs to silently fix the flags when they are > wrong. Would that be any better? Probably not. I'll take the original patches now and see if there is any complaints by users. thanks, greg k-h
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c index 263dd2f309fb..244417d0dfd1 100644 --- a/drivers/usb/core/devio.c +++ b/drivers/usb/core/devio.c @@ -1697,6 +1697,11 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb u |= URB_NO_INTERRUPT; as->urb->transfer_flags = u; + if (!allow_short && uurb->flags & USBDEVFS_URB_SHORT_NOT_OK) + dev_warn(&ps->dev->dev, "Requested nonsensical USBDEVFS_URB_SHORT_NOT_OK.\n"); + if (!allow_zero && uurb->flags & USBDEVFS_URB_ZERO_PACKET) + dev_warn(&ps->dev->dev, "Requested nonsensical USBDEVFS_URB_ZERO_PACKET.\n"); + as->urb->transfer_buffer_length = uurb->buffer_length; as->urb->setup_packet = (unsigned char *)dr; dr = NULL;
If we filter flags before they reach the core we need to generate our own warnings. Signed-off-by: Oliver Neukum <oneukum@suse.com> Fixes: 0cb54a3e47cb ("USB: debugging code shouldn't alter control flow") --- drivers/usb/core/devio.c | 5 +++++ 1 file changed, 5 insertions(+)