linux-kernel - Re: drivers/usb/usbip/stub_rx.c:505 stub_recv_cmd_submit() error: uninitialized symbol 'nents'.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <82478914-2bed-d8d8-0ee2-0460081434db@kernel.org>
Date:   Mon, 4 Nov 2019 10:07:28 -0700
From:   shuah <shuah@...nel.org>
To:     Suwan Kim <suwan.kim027@...il.com>
Cc:     Dan Carpenter <dan.carpenter@...cle.com>, kbuild@...ts.01.org,
        kbuild-all@...ts.01.org, linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-usb@...r.kernel.org, shuah <shuah@...nel.org>
Subject: Re: drivers/usb/usbip/stub_rx.c:505 stub_recv_cmd_submit() error:
 uninitialized symbol 'nents'.

On 11/1/19 8:34 AM, Suwan Kim wrote:
> On Tue, Oct 29, 2019 at 05:07:58AM -0600, shuah wrote:
>> On 10/25/19 9:40 PM, Suwan Kim wrote:

>> under this  check?
> 
> I understood. Moving this check after sgl_alloc() does not seem to
> require any additional checks on nents.
> 
> But I think we need to check for the case that Smatch reported that
> use_sg is true and buf_len is zero.
> 
> If there is no error check and an error condition occurs, the URB
> will be passed to the next step without a buffer.

Yes buf_len needs checking.

> 
> I attached the code. If you are okay, I will send a patch.
> 

This code looks good. Couple of comments.

> ---
> diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
> index 66edfeea68fe..0b6c4736ffd6 100644
> --- a/drivers/usb/usbip/stub_rx.c
> +++ b/drivers/usb/usbip/stub_rx.c
> @@ -476,12 +476,39 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
> 
>          buf_len = (unsigned long long)pdu->u.cmd_submit.transfer_buffer_length;
> 
> +       if (use_sg && !buf_len) {
> +               dev_err(&udev->dev, "sg buffer with zero length\n");
> +               goto err_malloc;

This is fine, what happens to the  priv allocated by stub_priv_alloc()?
Shouldn't that be released?

Can you add a comment above stub_priv_alloc() indicating that it adds
SDEV_EVENT_ERROR_MALLOC?

> +       }
> +
>          /* allocate urb transfer buffer, if needed */
>          if (buf_len) {
>                  if (use_sg) {
>                          sgl = sgl_alloc(buf_len, GFP_KERNEL, &nents);
>                          if (!sgl)
>                                  goto err_malloc;
> +
> +                       /* Check if the server's HCD supports SG */
> +                       if (!udev->bus->sg_tablesize) {
> +                               /*
> +                                * If the server's HCD doesn't support SG, break
> +                                * a single SG request into several URBs and map
> +                                * each SG list entry to corresponding URB
> +                                * buffer. The previously allocated SG list is
> +                                * stored in priv->sgl (If the server's HCD
> +                                * support SG, SG list is stored only in
> +                                * urb->sg) and it is used as an indicator that
> +                                * the server split single SG request into
> +                                * several URBs. Later, priv->sgl is used by
> +                                * stub_complete() and stub_send_ret_submit() to
> +                                * reassemble the divied URBs.
> +                                */
> +                               support_sg = 0;
> +                               num_urbs = nents;
> +                               priv->completed_urbs = 0;
> +                               pdu->u.cmd_submit.transfer_flags &=
> +                                                               ~URB_DMA_MAP_SG;
> +                       }
>                  } else {
>                          buffer = kzalloc(buf_len, GFP_KERNEL);
>                          if (!buffer)
> @@ -489,24 +516,6 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
>                  }
>          }
> 
> -       /* Check if the server's HCD supports SG */
> -       if (use_sg && !udev->bus->sg_tablesize) {
> -               /*
> -                * If the server's HCD doesn't support SG, break a single SG
> -                * request into several URBs and map each SG list entry to
> -                * corresponding URB buffer. The previously allocated SG
> -                * list is stored in priv->sgl (If the server's HCD support SG,
> -                * SG list is stored only in urb->sg) and it is used as an
> -                * indicator that the server split single SG request into
> -                * several URBs. Later, priv->sgl is used by stub_complete() and
> -                * stub_send_ret_submit() to reassemble the divied URBs.
> -                */
> -               support_sg = 0;
> -               num_urbs = nents;
> -               priv->completed_urbs = 0;
> -               pdu->u.cmd_submit.transfer_flags &= ~URB_DMA_MAP_SG;
> -       }
> -
>          /* allocate urb array */
>          priv->num_urbs = num_urbs;
>          priv->urbs = kmalloc_array(num_urbs, sizeof(*priv->urbs), GFP_KERNEL);
> 

thanks,
-- Shuah