linux-kernel - Re: [PATCH] USB: serial: Fix possible memleak in keyspan_port

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: <YWg6fBsl/1ui2vcR@hovoldconsulting.com>
Date:   Thu, 14 Oct 2021 16:11:08 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Wang Hai <wanghai38@...wei.com>
Cc:     gregkh@...uxfoundation.org, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] USB: serial: Fix possible memleak in keyspan_port_probe()

On Thu, Oct 14, 2021 at 09:20:33PM +0800, Wang Hai wrote:
> I got memory leak as follows when doing fault injection test:
> 
> unreferenced object 0xffff888258228440 (size 64):
>   comm "kworker/7:2", pid 2005, jiffies 4294989509 (age 824.540s)
>   hex dump (first 32 bytes):
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   backtrace:
>     [<ffffffff8167939c>] slab_post_alloc_hook+0x9c/0x490
>     [<ffffffff8167f627>] kmem_cache_alloc_trace+0x1f7/0x470
>     [<ffffffffa02ac0e4>] keyspan_port_probe+0xa4/0x5d0 [keyspan]
>     [<ffffffffa0294c07>] usb_serial_device_probe+0x97/0x1d0 [usbserial]
>     [<ffffffff82b50ca7>] really_probe+0x167/0x460
>     [<ffffffff82b51099>] __driver_probe_device+0xf9/0x180
>     [<ffffffff82b51173>] driver_probe_device+0x53/0x130
>     [<ffffffff82b516f5>] __device_attach_driver+0x105/0x130
>     [<ffffffff82b4cfe9>] bus_for_each_drv+0x129/0x190
>     [<ffffffff82b50a69>] __device_attach+0x1c9/0x270
>     [<ffffffff82b518d0>] device_initial_probe+0x20/0x30
>     [<ffffffff82b4f062>] bus_probe_device+0x142/0x160
>     [<ffffffff82b4a4e9>] device_add+0x829/0x1300
>     [<ffffffffa0295fda>] usb_serial_probe.cold+0xc9b/0x14ac [usbserial]
>     [<ffffffffa02266aa>] usb_probe_interface+0x1aa/0x3c0 [usbcore]
>     [<ffffffff82b50ca7>] really_probe+0x167/0x460
> 
> If it fails to allocate memory for an out_buffer[i] or in_buffer[i],
> the previously allocated memory for out_buffer or in_buffer needs to
> be freed on the error handling path, otherwise a memory leak will result.
> 
> Fixes: bad41a5bf177 ("USB: keyspan: fix port DMA-buffer allocations")
> Reported-by: Hulk Robot <hulkci@...wei.com>
> Signed-off-by: Wang Hai <wanghai38@...wei.com>
> ---
>  drivers/usb/serial/keyspan.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/serial/keyspan.c b/drivers/usb/serial/keyspan.c
> index 87b89c99d517..ba27a9f0275b 100644
> --- a/drivers/usb/serial/keyspan.c
> +++ b/drivers/usb/serial/keyspan.c
> @@ -2901,7 +2901,7 @@ static int keyspan_port_probe(struct usb_serial_port *port)
>  
>  	p_priv->inack_buffer = kzalloc(INACK_BUFLEN, GFP_KERNEL);
>  	if (!p_priv->inack_buffer)
> -		goto err_inack_buffer;
> +		goto err_out_buffer;
>  
>  	p_priv->outcont_buffer = kzalloc(OUTCONT_BUFLEN, GFP_KERNEL);
>  	if (!p_priv->outcont_buffer)
> @@ -2953,13 +2953,12 @@ static int keyspan_port_probe(struct usb_serial_port *port)
>  
>  err_outcont_buffer:
>  	kfree(p_priv->inack_buffer);
> -err_inack_buffer:
> +err_out_buffer:
>  	for (i = 0; i < ARRAY_SIZE(p_priv->out_buffer); ++i)
>  		kfree(p_priv->out_buffer[i]);
> -err_out_buffer:
> +err_in_buffer:
>  	for (i = 0; i < ARRAY_SIZE(p_priv->in_buffer); ++i)
>  		kfree(p_priv->in_buffer[i]);
> -err_in_buffer:
>  	kfree(p_priv);
>  
>  	return -ENOMEM;

Good catch. Fortunately these small allocations would currently never
fail, but we should fix it up nonetheless.

The fix looks correct, but you're now mixing two styles of error labels
(i.e. naming them after where you jump from and after what they do,
respectively).

Since you're touching all but one label, could you rename also the last
one after what is done and include a "free_" infix in the label names
(e.g. err_free_in_buffer, etc)?

Johan