netdev - Re: [PATCH bpf-next v3] xsk: fix xsk_build_skb() error: 'skb' dereferencing possible ERR

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: <CAJ8uoz26kQbpqikaWkO3TNwMC=4V=96tppGcm0fjpXgfRkKUww@mail.gmail.com>
Date: Fri, 25 Aug 2023 20:53:55 +0200
From: Magnus Karlsson <magnus.karlsson@...il.com>
To: Tirthendu Sarkar <tirthendu.sarkar@...el.com>
Cc: bpf@...r.kernel.org, netdev@...r.kernel.org, bjorn@...nel.org, 
	magnus.karlsson@...el.com, maciej.fijalkowski@...el.com, 
	jonathan.lemon@...il.com, davem@...emloft.net, kuba@...nel.org, 
	pabeni@...hat.com, ast@...nel.org, daniel@...earbox.net, martin.lau@...ux.dev, 
	dan.carpenter@...aro.org, sdf@...gle.com
Subject: Re: [PATCH bpf-next v3] xsk: fix xsk_build_skb() error: 'skb'
 dereferencing possible ERR_PTR()

On Wed, 23 Aug 2023 at 17:05, Tirthendu Sarkar
<tirthendu.sarkar@...el.com> wrote:
>
> Currently, xsk_build_skb() is a function that builds skb in two possible
> ways and then is ended with common error handling.
>
> We can distinguish four possible error paths and handling in xsk_build_skb():
>  1. sock_alloc_send_skb fails: Retry (skb is NULL).
>  2. skb_store_bits fails : Free skb and retry.
>  3. MAX_SKB_FRAGS exceeded: Free skb, cleanup and drop packet.
>  4. alloc_page fails for frag: Retry page allocation w/o freeing skb
>
> 1] and 3] can happen in xsk_build_skb_zerocopy(), which is one of the two
> code paths responsible for building skb. Common error path in
> xsk_build_skb() assumes that in case errno != -EAGAIN, skb is a valid
> pointer, which is wrong as kernel test robot reports that in
> xsk_build_skb_zerocopy() other errno values are returned for skb being
> NULL.
>
> To fix this, set -EOVERFLOW as error when MAX_SKB_FRAGS are exceeded and
> packet needs to be dropped in both xsk_build_skb() and
> xsk_build_skb_zerocopy() and use this to distinguish against all other
> error cases. Also, add explicit kfree_skb() for 3] so that handling of 1],
> 2], and 3] becomes identical where allocation needs to be retried.

Thanks Tirtha for the fix.

Acked-by: Magnus Karlsson <magnus.karlsson@...el.com>

> Signed-off-by: Tirthendu Sarkar <tirthendu.sarkar@...el.com>
> Reported-by: kernel test robot <lkp@...el.com>
> Reported-by: Dan Carpenter <dan.carpenter@...aro.org>
> Closes: https://lore.kernel.org/r/202307210434.OjgqFcbB-lkp@intel.com/
> Fixes: cf24f5a5feea ("xsk: add support for AF_XDP multi-buffer on Tx path")
> ---
> Changelog:
>         v2 -> v3:
>         - Added further details in commit message as asked by Maciej
>         v1 -> v2:
>         - Removed err as a parameter to xsk_build_skb_zerocopy()
>         [Stanislav Fomichev]
>         - use explicit error to distinguish packet drop vs retry
>
>  net/xdp/xsk.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index fcfc8472f73d..55f8b9b0e06d 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -602,7 +602,7 @@ static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,
>
>         for (copied = 0, i = skb_shinfo(skb)->nr_frags; copied < len; i++) {
>                 if (unlikely(i >= MAX_SKB_FRAGS))
> -                       return ERR_PTR(-EFAULT);
> +                       return ERR_PTR(-EOVERFLOW);
>
>                 page = pool->umem->pgs[addr >> PAGE_SHIFT];
>                 get_page(page);
> @@ -655,15 +655,17 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
>                         skb_put(skb, len);
>
>                         err = skb_store_bits(skb, 0, buffer, len);
> -                       if (unlikely(err))
> +                       if (unlikely(err)) {
> +                               kfree_skb(skb);
>                                 goto free_err;
> +                       }
>                 } else {
>                         int nr_frags = skb_shinfo(skb)->nr_frags;
>                         struct page *page;
>                         u8 *vaddr;
>
>                         if (unlikely(nr_frags == (MAX_SKB_FRAGS - 1) && xp_mb_desc(desc))) {
> -                               err = -EFAULT;
> +                               err = -EOVERFLOW;
>                                 goto free_err;
>                         }
>
> @@ -690,12 +692,14 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
>         return skb;
>
>  free_err:
> -       if (err == -EAGAIN) {
> -               xsk_cq_cancel_locked(xs, 1);
> -       } else {
> -               xsk_set_destructor_arg(skb);
> -               xsk_drop_skb(skb);
> +       if (err == -EOVERFLOW) {
> +               /* Drop the packet */
> +               xsk_set_destructor_arg(xs->skb);
> +               xsk_drop_skb(xs->skb);
>                 xskq_cons_release(xs->tx);
> +       } else {
> +               /* Let application retry */
> +               xsk_cq_cancel_locked(xs, 1);
>         }
>
>         return ERR_PTR(err);
> @@ -738,7 +742,7 @@ static int __xsk_generic_xmit(struct sock *sk)
>                 skb = xsk_build_skb(xs, &desc);
>                 if (IS_ERR(skb)) {
>                         err = PTR_ERR(skb);
> -                       if (err == -EAGAIN)
> +                       if (err != -EOVERFLOW)
>                                 goto out;
>                         err = 0;
>                         continue;
> --
> 2.34.1
>
>