netdev - Re: [bpf-next,02/11] xsk: introduce xdp_umem

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: <c1cb2ca8-6a14-3980-8672-f3de0bb38dfd@suse.cz>
Date:   Wed, 13 Mar 2019 10:39:49 +0100
From:   Jiri Slaby <jslaby@...e.cz>
To:     Björn Töpel <bjorn.topel@...il.com>,
        magnus.karlsson@...el.com, magnus.karlsson@...il.com,
        alexander.h.duyck@...el.com, alexander.duyck@...il.com, ast@...com,
        brouer@...hat.com, daniel@...earbox.net, netdev@...r.kernel.org,
        mykyta.iziumtsev@...aro.org
Cc:     Björn Töpel <bjorn.topel@...el.com>,
        john.fastabend@...il.com, willemdebruijn.kernel@...il.com,
        mst@...hat.com, michael.lundkvist@...csson.com,
        jesse.brandeburg@...el.com, anjali.singhai@...el.com,
        qi.z.zhang@...el.com, francois.ozog@...aro.org,
        ilias.apalodimas@...aro.org, brian.brooks@...aro.org,
        andy@...yhouse.net, michael.chan@...adcom.com,
        intel-wired-lan@...ts.osuosl.org
Subject: Re: [bpf-next,02/11] xsk: introduce xdp_umem_page

On 04. 06. 18, 14:05, Björn Töpel wrote:
> From: Björn Töpel <bjorn.topel@...el.com>
> 
> The xdp_umem_page holds the address for a page. Trade memory for
> faster lookup. Later, we'll add DMA address here as well.
...
> --- a/include/net/xdp_sock.h
> +++ b/include/net/xdp_sock.h
...
> --- a/net/xdp/xdp_umem.c
> +++ b/net/xdp/xdp_umem.c
> @@ -65,6 +65,9 @@ static void xdp_umem_release(struct xdp_umem *umem)
>  		goto out;
>  
>  	mmput(mm);
> +	kfree(umem->pages);
> +	umem->pages = NULL;
> +

Are you sure about the placement of kfree here? Why is it dependent on
task && mm above?

IMO the kfree should be below "out:":

>  	xdp_umem_unaccount_pages(umem);
>  out:
>  	kfree(umem);
Syzkaller reported this memleak:
r0 = socket$xdp(0x2c, 0x3, 0x0)
setsockopt$XDP_UMEM_REG(r0, 0x11b, 0x4,
&(0x7f0000000100)={&(0x7f0000000000)=""/210, 0x20000, 0x1000, 0x7}, 0x18)
BUG: memory leak
unreferenced object 0xffff88003648de68 (size 512):
  comm "syz-executor.3", pid 11688, jiffies 4295555546 (age 15.752s)
  hex dump (first 32 bytes):
    00 00 40 23 00 88 ff ff 00 00 00 00 00 00 00 00  ..@.............
    00 10 40 23 00 88 ff ff 00 00 00 00 00 00 00 00  ..@.............
  backtrace:
    [<ffffffffa9f8346c>] xsk_setsockopt+0x40c/0x510 net/xdp/xsk.c:539
    [<ffffffffa9935c41>] SyS_setsockopt+0x171/0x370 net/socket.c:1862
    [<ffffffffa800b28c>] do_syscall_64+0x26c/0x6e0
arch/x86/entry/common.c:284
    [<ffffffffaa00009a>] entry_SYSCALL_64_after_hwframe+0x3d/0xa2
    [<ffffffffffffffff>] 0xffffffffffffffff


Given the size of the leak, it looks like umem->pages is leaked:
mr->len/page_size*sizeof(*umem->pages)
0x20000/4096*16=512

So I added a check, and really, task is NULL in my testcase -- the
program is gone when the deferred work triggers. But umem->pages is not
freed.

Moving the free after "out:", no leaks happen anymore.

Easily reproducible with:
#include <err.h>
#include <stdlib.h>
#include <unistd.h>

#include <sys/socket.h>
#include <sys/types.h>
#include <sys/wait.h>

#include <linux/if_xdp.h>

void fun()
{
        static char buffer[0x20000] __attribute__((aligned(4096)));
        struct xdp_umem_reg mr = {
                (unsigned long)buffer,
                0x20000,
                0x1000,
                0x7,
         //&(0x7f0000000100)={&(0x7f0000000000)=""/210, 0x20000, 0x1000,
0x7}
        };
        int r0;
        r0 = socket(AF_XDP, SOCK_RAW, 0);
        if (r0 < 0)
                err(1, "socket");
        if (setsockopt(r0, SOL_XDP, XDP_UMEM_REG, &mr, sizeof(mr)) < 0)
                err(1, "setsockopt");
        close(r0);
}

int main()
{
        int a;
        while (1) {
                for (a = 0; a < 40; a++)
                        if (!fork()) {
                                fun();
                                exit(0);
                        }
                for (a = 0; a < 100; a++)
                        wait(NULL);
        }
}


thanks,
-- 
js
suse labs