[alsa-devel] [PATCH 2/9] ALSA: bcd2000: Add a sanity check for invalid EPs

[Andrey Konovalov]

On Tue, Oct 10, 2017 at 4:33 PM, Takashi Iwai <tiwai at suse.de> wrote:
> On Tue, 10 Oct 2017 16:00:25 +0200,
> Andrey Konovalov wrote:
>>
>> On Tue, Oct 10, 2017 at 3:38 PM, Takashi Iwai <tiwai at suse.de> wrote:
>> > As syzkaller spotted, currently bcd2000 driver submits a URB with the
>> > fixed EP without checking whether it's actually available, which may
>> > result in a kernel warning like:
>> >   usb 1-1: BOGUS urb xfer, pipe 1 != type 3
>> >   ------------[ cut here ]------------
>> >   WARNING: CPU: 0 PID: 1846 at drivers/usb/core/urb.c:449
>> >   usb_submit_urb+0xf8a/0x11d0
>> >   Modules linked in:
>> >   CPU: 0 PID: 1846 Comm: kworker/0:2 Not tainted
>> >   4.14.0-rc2-42613-g1488251d1a98 #238
>> >   Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>> >   Workqueue: usb_hub_wq hub_event
>> >   Call Trace:
>> >    bcd2000_init_device sound/usb/bcd2000/bcd2000.c:289
>> >    bcd2000_init_midi sound/usb/bcd2000/bcd2000.c:345
>> >    bcd2000_probe+0xe64/0x19e0 sound/usb/bcd2000/bcd2000.c:406
>> >    usb_probe_interface+0x35d/0x8e0 drivers/usb/core/driver.c:361
>> >    ....
>> >
>> > This patch adds a sanity check of validity of EPs at the device
>> > initialization phase for avoiding the call with an invalid EP.
>> >
>> > Reported-by: Andrey Konovalov <andreyknvl at google.com>
>> > Signed-off-by: Takashi Iwai <tiwai at suse.de>
>>
>> Hi Takashi,
>>
>> I've applied patches #1 and #2 and for some reason get this when I try
>> to build the kernel:
>>
>>   LD      vmlinux.o
>>   MODPOST vmlinux.o
>> sound/usb/bcd2000/bcd2000.o: In function `bcd2000_init_midi':
>> .../sound/usb/bcd2000/bcd2000.c:346: undefined reference to
>> `usb_urb_ep_type_check'
>> .../sound/usb/bcd2000/bcd2000.c:347: undefined reference to
>> `usb_urb_ep_type_check'
>> make: *** [vmlinux] Error 1
>>
>> What could be wrong?
>
> Mea culpa, I generated patches from the wrong branch.
> Luckily only the first patch was wrong, the function name was
> misspelled.

Ah, I thought so and even intentionally checked for a typo in the
function name, but somehow still missed that :)

I've run my reproducers with your patches applied, all the warnings are gone.

Thanks!

Tested-by: Andrey Konovalov <andreyknvl at google.com>

>
> Below is the right patch for patch 1, which already includes Greg's
> suggestions.  I'm going to send a v2 series in anyway later, so just
> putting this one below.
>
> Sorry for the inconvenience!
>
>
> Takashi
>
> -- 8< --
> From: Takashi Iwai <tiwai at suse.de>
> Subject: [PATCH 1/9] usb: core: Add a helper function to check the validity of EP type in URB
>
> This patch adds a new helper function to perform a sanity check of the
> given URB to see whether it contains a valid endpoint.  It's a light-
> weight version of what usb_submit_urb() does, but without the kernel
> warning followed by the stack trace, just returns an error code.
>
> Especially for a driver that doesn't parse the descriptor but fills
> the URB with the fixed endpoint (e.g. some quirks for non-compliant
> devices), this kind of check is preferable at the probe phase before
> actually submitting the urb.
>
> Signed-off-by: Takashi Iwai <tiwai at suse.de>
> ---
>  drivers/usb/core/urb.c | 30 ++++++++++++++++++++++++++----
>  include/linux/usb.h    |  2 ++
>  2 files changed, 28 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> index 47903d510955..8b800e34407b 100644
> --- a/drivers/usb/core/urb.c
> +++ b/drivers/usb/core/urb.c
> @@ -187,6 +187,31 @@ EXPORT_SYMBOL_GPL(usb_unanchor_urb);
>
>  /*-------------------------------------------------------------------*/
>
> +static const int pipetypes[4] = {
> +       PIPE_CONTROL, PIPE_ISOCHRONOUS, PIPE_BULK, PIPE_INTERRUPT
> +};
> +
> +/**
> + * usb_urb_ep_type_check - sanity check of endpoint in the given urb
> + * @urb: urb to be checked
> + *
> + * This performs a light-weight sanity check for the endpoint in the
> + * given urb.  It returns 0 if the urb contains a valid endpoint, otherwise
> + * a negative error code.
> + */
> +int usb_urb_ep_type_check(const struct urb *urb)
> +{
> +       const struct usb_host_endpoint *ep;
> +
> +       ep = usb_pipe_endpoint(urb->dev, urb->pipe);
> +       if (!ep)
> +               return -EINVAL;
> +       if (usb_pipetype(urb->pipe) != pipetypes[usb_endpoint_type(&ep->desc)])
> +               return -EINVAL;
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_urb_ep_type_check);
> +
>  /**
>   * usb_submit_urb - issue an asynchronous transfer request for an endpoint
>   * @urb: pointer to the urb describing the request
> @@ -326,9 +351,6 @@ EXPORT_SYMBOL_GPL(usb_unanchor_urb);
>   */
>  int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
>  {
> -       static int                      pipetypes[4] = {
> -               PIPE_CONTROL, PIPE_ISOCHRONOUS, PIPE_BULK, PIPE_INTERRUPT
> -       };
>         int                             xfertype, max;
>         struct usb_device               *dev;
>         struct usb_host_endpoint        *ep;
> @@ -444,7 +466,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_pipetype(urb->pipe) != pipetypes[xfertype])
> +       if (usb_urb_ep_type_check(urb))
>                 dev_WARN(&dev->dev, "BOGUS urb xfer, pipe %x != type %x\n",
>                         usb_pipetype(urb->pipe), pipetypes[xfertype]);
>
> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index cb9fbd54386e..2b861804fffa 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -1728,6 +1728,8 @@ static inline int usb_urb_dir_out(struct urb *urb)
>         return (urb->transfer_flags & URB_DIR_MASK) == URB_DIR_OUT;
>  }
>
> +int usb_urb_ep_type_check(const struct urb *urb);
> +
>  void *usb_alloc_coherent(struct usb_device *dev, size_t size,
>         gfp_t mem_flags, dma_addr_t *dma);
>  void usb_free_coherent(struct usb_device *dev, size_t size,
> --
> 2.14.2
>