netdev - Re: [PATCH bpf 1/2] libbpf: fix invalid munmap call

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] [day] [month] [year] [list]

Message-ID: <ea7d7ea7-a2cb-0ed7-2fa3-9d8db5b62381@iogearbox.net>
Date:   Mon, 6 May 2019 11:01:58 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Björn Töpel <bjorn.topel@...il.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Netdev <netdev@...r.kernel.org>,
        Björn Töpel <bjorn.topel@...el.com>,
        "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        Magnus Karlsson <magnus.karlsson@...il.com>,
        bpf <bpf@...r.kernel.org>, William Tu <u9012063@...il.com>
Subject: Re: [PATCH bpf 1/2] libbpf: fix invalid munmap call

On 05/06/2019 10:40 AM, Björn Töpel wrote:
> On Mon, 6 May 2019 at 10:26, Daniel Borkmann <daniel@...earbox.net> wrote:
>> On 04/30/2019 02:45 PM, Björn Töpel wrote:
>>> From: Björn Töpel <bjorn.topel@...el.com>
>>>
>>> When unmapping the AF_XDP memory regions used for the rings, an
>>> invalid address was passed to the munmap() calls. Instead of passing
>>> the beginning of the memory region, the descriptor region was passed
>>> to munmap.
>>>
>>> When the userspace application tried to tear down an AF_XDP socket,
>>> the operation failed and the application would still have a reference
>>> to socket it wished to get rid of.
>>>
>>> Reported-by: William Tu <u9012063@...il.com>
>>> Fixes: 1cad07884239 ("libbpf: add support for using AF_XDP sockets")
>>> Signed-off-by: Björn Töpel <bjorn.topel@...el.com>
>> [...]
>>>  out_mmap_tx:
>>>       if (tx)
>>> -             munmap(xsk->tx,
>>> -                    off.tx.desc +
>>> +             munmap(tx_map, off.tx.desc +
>>>                      xsk->config.tx_size * sizeof(struct xdp_desc));
>>>  out_mmap_rx:
>>>       if (rx)
>>> -             munmap(xsk->rx,
>>> -                    off.rx.desc +
>>> +             munmap(rx_map, off.rx.desc +
>>>                      xsk->config.rx_size * sizeof(struct xdp_desc));
>>>  out_socket:
>>>       if (--umem->refcount)
>>> @@ -684,10 +681,12 @@ int xsk_umem__delete(struct xsk_umem *umem)
>>>       optlen = sizeof(off);
>>>       err = getsockopt(umem->fd, SOL_XDP, XDP_MMAP_OFFSETS, &off, &optlen);
>>>       if (!err) {
>>> -             munmap(umem->fill->ring,
>>> -                    off.fr.desc + umem->config.fill_size * sizeof(__u64));
>>> -             munmap(umem->comp->ring,
>>> -                    off.cr.desc + umem->config.comp_size * sizeof(__u64));
>>> +             (void)munmap(umem->fill->ring - off.fr.desc,
>>> +                          off.fr.desc +
>>> +                          umem->config.fill_size * sizeof(__u64));
>>> +             (void)munmap(umem->comp->ring - off.cr.desc,
>>> +                          off.cr.desc +
>>> +                          umem->config.comp_size * sizeof(__u64));
>>
>> What's the rationale to cast to void here and other places (e.g. below)?
>> If there's no proper reason, then lets remove it. Given the patch has already
>> been applied, please send a follow up. Thanks.
> 
> The rationale was to make it explicit that the return value is *not*
> cared about. If this is not common practice, I'll remove it in a
> follow up!

Yeah, it's not common practice, so lets rather remove it.

Thanks,
Daniel