linux-kernel - Re: [PATCH] staging: ft1000: fix memory leak on error path in ft1000

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: <CAAfyv348JfJs-FbL3zvQgQ1i=MONVJWdhgfmqzdYOvGxcPAK0Q@mail.gmail.com>
Date:	Mon, 10 Jun 2013 22:40:19 +0200
From:	Belisko Marek <marek.belisko@...il.com>
To:	Alexey Khoroshilov <khoroshilov@...ras.ru>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	devel@...verdev.osuosl.org, LKML <linux-kernel@...r.kernel.org>,
	ldv-project@...uxtesting.org
Subject: Re: [PATCH] staging: ft1000: fix memory leak on error path in ft1000_probe()

Hi Alexey,

On Mon, Jun 10, 2013 at 9:29 PM, Alexey Khoroshilov
<khoroshilov@...ras.ru> wrote:
> ft1000dev->tx_urb and ft1000dev->rx_urb are not deallocated
> if something goes wrong in ft1000_probe(). Also there is no
> check for success of urb allocation. The patch fixes the both issues.
>
> By the way, there is no sense in GFP_ATOMIC for urb allocation here,
> so it is changed to GFP_KERNEL.
>
> Found by Linux Driver Verification project (linuxtesting.org).
>
> Signed-off-by: Alexey Khoroshilov <khoroshilov@...ras.ru>
> ---
>  drivers/staging/ft1000/ft1000-usb/ft1000_usb.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/ft1000/ft1000-usb/ft1000_usb.c b/drivers/staging/ft1000/ft1000-usb/ft1000_usb.c
> index 614db55..29a7cd2 100644
> --- a/drivers/staging/ft1000/ft1000-usb/ft1000_usb.c
> +++ b/drivers/staging/ft1000/ft1000-usb/ft1000_usb.c
> @@ -79,8 +79,12 @@ static int ft1000_probe(struct usb_interface *interface,
>         ft1000dev->dev = dev;
>         ft1000dev->status = 0;
>         ft1000dev->net = NULL;
> -       ft1000dev->tx_urb = usb_alloc_urb(0, GFP_ATOMIC);
> -       ft1000dev->rx_urb = usb_alloc_urb(0, GFP_ATOMIC);
> +       ft1000dev->tx_urb = usb_alloc_urb(0, GFP_KERNEL);
Can we check return value for tx here? If allocation fails it makes no sense to
try allocate also rx. Otherwise looks good. Thanks.
> +       ft1000dev->rx_urb = usb_alloc_urb(0, GFP_KERNEL);
Same for rx here.
> +       if (!ft1000dev->tx_urb || !ft1000dev->rx_urb) {
> +               ret = -ENOMEM;
> +               goto err_fw;
> +       }
>
>         DEBUG("ft1000_probe is called\n");
>         numaltsetting = interface->num_altsetting;
> @@ -209,6 +213,8 @@ err_thread:
>  err_load:
>         kfree(pFileStart);
>  err_fw:
> +       usb_free_urb(ft1000dev->rx_urb);
> +       usb_free_urb(ft1000dev->tx_urb);
>         kfree(ft1000dev);
>         return ret;
>  }
> --
> 1.8.1.2
>

marek

--
as simple and primitive as possible
-------------------------------------------------
Marek Belisko - OPEN-NANDRA
Freelance Developer

Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
Tel: +421 915 052 184
skype: marekwhite
twitter: #opennandra
web: http://open-nandra.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/