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

Message-ID: <CAJ+HfNhuHoZ5HEBTOTapg9Mu2vMB1pFvbXXrXbkRo4E7m2nNLQ@mail.gmail.com>
Date:   Mon, 6 May 2019 10:40:39 +0200
From:   Björn Töpel <bjorn.topel@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
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 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!

Thank,
Björn

> >       }
> >
> >       close(umem->fd);
> > @@ -698,6 +697,7 @@ int xsk_umem__delete(struct xsk_umem *umem)
> >
> >  void xsk_socket__delete(struct xsk_socket *xsk)
> >  {
> > +     size_t desc_sz = sizeof(struct xdp_desc);
> >       struct xdp_mmap_offsets off;
> >       socklen_t optlen;
> >       int err;
> > @@ -710,14 +710,17 @@ void xsk_socket__delete(struct xsk_socket *xsk)
> >       optlen = sizeof(off);
> >       err = getsockopt(xsk->fd, SOL_XDP, XDP_MMAP_OFFSETS, &off, &optlen);
> >       if (!err) {
> > -             if (xsk->rx)
> > -                     munmap(xsk->rx->ring,
> > -                            off.rx.desc +
> > -                            xsk->config.rx_size * sizeof(struct xdp_desc));
> > -             if (xsk->tx)
> > -                     munmap(xsk->tx->ring,
> > -                            off.tx.desc +
> > -                            xsk->config.tx_size * sizeof(struct xdp_desc));
> > +             if (xsk->rx) {
> > +                     (void)munmap(xsk->rx->ring - off.rx.desc,
> > +                                  off.rx.desc +
> > +                                  xsk->config.rx_size * desc_sz);
> > +             }
> > +             if (xsk->tx) {
> > +                     (void)munmap(xsk->tx->ring - off.tx.desc,
> > +                                  off.tx.desc +
> > +                                  xsk->config.tx_size * desc_sz);
> > +             }
> > +
> >       }
> >
> >       xsk->umem->refcount--;
> >
>