netdev - Re: [PATCH bpf v2] xsk: fix broken Tx ring validation

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+HfNhP48y=zpzjr60hbp4iJzBFTWG9-JaBf0bQcSsAW71xTQ@mail.gmail.com>
Date:   Fri, 18 Jun 2021 15:33:58 +0200
From:   Björn Töpel <bjorn.topel@...il.com>
To:     Magnus Karlsson <magnus.karlsson@...il.com>
Cc:     "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Netdev <netdev@...r.kernel.org>,
        "Fijalkowski, Maciej" <maciej.fijalkowski@...el.com>,
        Jonathan Lemon <jonathan.lemon@...il.com>,
        bpf <bpf@...r.kernel.org>, Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
Subject: Re: [PATCH bpf v2] xsk: fix broken Tx ring validation

On Fri, 18 Jun 2021 at 09:58, Magnus Karlsson <magnus.karlsson@...il.com> wrote:
>
> From: Magnus Karlsson <magnus.karlsson@...el.com>
>
> Fix broken Tx ring validation for AF_XDP. The commit under the Fixes
> tag, fixed an off-by-one error in the validation but introduced
> another error. Descriptors are now let through even if they straddle a
> chunk boundary which they are not allowed to do in aligned mode. Worse
> is that they are let through even if they straddle the end of the umem
> itself, tricking the kernel to read data outside the allowed umem
> region which might or might not be mapped at all.
>
> Fix this by reintroducing the old code, but subtract the length by one
> to fix the off-by-one error that the original patch was
> addressing. The test chunk != chunk_end makes sure packets do not
> straddle chunk boundraries. Note that packets of zero length are
> allowed in the interface, therefore the test if the length is
> non-zero.
>
> v1 -> v2:
> * Improved commit message
>
> Fixes: ac31565c2193 ("xsk: Fix for xp_aligned_validate_desc() when len == chunk_size")
> Reviewed-by: Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
> Signed-off-by: Magnus Karlsson <magnus.karlsson@...el.com>

Acked-by: Björn Töpel <bjorn@...nel.org>

> ---
>  net/xdp/xsk_queue.h | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/net/xdp/xsk_queue.h b/net/xdp/xsk_queue.h
> index 9d2a89d793c0..9ae13cccfb28 100644
> --- a/net/xdp/xsk_queue.h
> +++ b/net/xdp/xsk_queue.h
> @@ -128,12 +128,15 @@ static inline bool xskq_cons_read_addr_unchecked(struct xsk_queue *q, u64 *addr)
>  static inline bool xp_aligned_validate_desc(struct xsk_buff_pool *pool,
>                                             struct xdp_desc *desc)
>  {
> -       u64 chunk;
> -
> -       if (desc->len > pool->chunk_size)
> -               return false;
> +       u64 chunk, chunk_end;
>
>         chunk = xp_aligned_extract_addr(pool, desc->addr);
> +       if (likely(desc->len)) {
> +               chunk_end = xp_aligned_extract_addr(pool, desc->addr + desc->len - 1);
> +               if (chunk != chunk_end)
> +                       return false;
> +       }
> +
>         if (chunk >= pool->addrs_cnt)
>                 return false;
>
>
> base-commit: da5ac772cfe2a03058b0accfac03fad60c46c24d
> --
> 2.29.0
>