netdev - Re: [PATCH] nfc: virtual_ncidev: Fix memory leak in virtual_nci

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: <bb57c67a9235cb47d62a7cf8c01b70b8815b2d29.camel@redhat.com>
Date:   Tue, 18 Oct 2022 11:32:18 +0200
From:   Paolo Abeni <pabeni@...hat.com>
To:     Shang XiaoJing <shangxiaojing@...wei.com>, bongsu.jeon@...sung.com,
        krzysztof.kozlowski@...aro.org, kuba@...nel.org,
        netdev@...r.kernel.org
Subject: Re: [PATCH] nfc: virtual_ncidev: Fix memory leak in
 virtual_nci_send()

Hello,

On Mon, 2022-10-17 at 16:44 +0800, Shang XiaoJing wrote:
> skb should be free in virtual_nci_send(), otherwise kmemleak will report
> memleak.
> 
> Steps for reproduction (simulated in qemu):
> 	cd tools/testing/selftests/nci
> 	make
> 	./nci_dev
> 
> BUG: memory leak
> unreferenced object 0xffff888107588000 (size 208):
>   comm "nci_dev", pid 206, jiffies 4294945376 (age 368.248s)
>   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:
>     [<000000008d94c8fd>] __alloc_skb+0x1da/0x290
>     [<00000000278bc7f8>] nci_send_cmd+0xa3/0x350
>     [<0000000081256a22>] nci_reset_req+0x6b/0xa0
>     [<000000009e721112>] __nci_request+0x90/0x250
>     [<000000005d556e59>] nci_dev_up+0x217/0x5b0
>     [<00000000e618ce62>] nfc_dev_up+0x114/0x220
>     [<00000000981e226b>] nfc_genl_dev_up+0x94/0xe0
>     [<000000009bb03517>] genl_family_rcv_msg_doit.isra.14+0x228/0x2d0
>     [<00000000b7f8c101>] genl_rcv_msg+0x35c/0x640
>     [<00000000c94075ff>] netlink_rcv_skb+0x11e/0x350
>     [<00000000440cfb1e>] genl_rcv+0x24/0x40
>     [<0000000062593b40>] netlink_unicast+0x43f/0x640
>     [<000000001d0b13cc>] netlink_sendmsg+0x73a/0xbf0
>     [<000000003272487f>] __sys_sendto+0x324/0x370
>     [<00000000ef9f1747>] __x64_sys_sendto+0xdd/0x1b0
>     [<000000001e437841>] do_syscall_64+0x3f/0x90
> 
> Fixes: e624e6c3e777 ("nfc: Add a virtual nci device driver")
> Signed-off-by: Shang XiaoJing <shangxiaojing@...wei.com>
> ---
>  drivers/nfc/virtual_ncidev.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/nfc/virtual_ncidev.c b/drivers/nfc/virtual_ncidev.c
> index f577449e4935..9f54a4e0eb51 100644
> --- a/drivers/nfc/virtual_ncidev.c
> +++ b/drivers/nfc/virtual_ncidev.c
> @@ -64,6 +64,7 @@ static int virtual_nci_send(struct nci_dev *ndev, struct sk_buff *skb)
>  	send_buff = skb_copy(skb, GFP_KERNEL);
>  	mutex_unlock(&nci_mutex);
>  	wake_up_interruptible(&wq);
> +	consume_skb(skb);

Looking at the nci core, it seems to me that the send op takes full
ownership of the skb argument. That is, you need to additionally free
it even on error paths.

@Krzysztof: it looks like that the send() return value is always
ignored. Do you plan to use it at some point or could we change the
return type to void?

Thanks,

Paolo