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: <20aaa3f5-fd93-9773-ca8a-40809e9dc981@iogearbox.net>
Date:   Mon, 6 May 2019 10:26:00 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Björn Töpel <bjorn.topel@...il.com>,
        ast@...nel.org, netdev@...r.kernel.org
Cc:     Björn Töpel <bjorn.topel@...el.com>,
        magnus.karlsson@...el.com, magnus.karlsson@...il.com,
        bpf@...r.kernel.org, u9012063@...il.com
Subject: Re: [PATCH bpf 1/2] libbpf: fix invalid munmap call

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.

>  	}
>  
>  	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--;
>