netdev - Re: [PATCH bpf] xsk: fix generic transmit when completion queue reservation fails

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]

Date:   Tue, 14 Jun 2022 15:16:01 +0200
From:   Magnus Karlsson <magnus.karlsson@...il.com>
To:     Ciara Loftus <ciara.loftus@...el.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>,
        Björn Töpel <bjorn@...nel.org>,
        "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        Jonathan Lemon <jonathan.lemon@...il.com>,
        "Fijalkowski, Maciej" <maciej.fijalkowski@...el.com>
Subject: Re: [PATCH bpf] xsk: fix generic transmit when completion queue
 reservation fails

On Tue, Jun 14, 2022 at 9:09 AM Ciara Loftus <ciara.loftus@...el.com> wrote:
>
> Two points of potential failure in the generic transmit function are:
> 1. completion queue (cq) reservation failure.
> 2. skb allocation failure
>
> Originally the cq reservation was performed first, followed by the skb
> allocation. Commit 675716400da6 ("xdp: fix possible cq entry leak")
> reversed the order because at the time there was no mechanism available to
> undo the cq reservation which could have led to possible cq entry leaks in
> the event of skb allocation failure. However if the skb allocation is
> performed first and the cq reservation then fails, the xsk skb destructor
> is called which blindly adds the skb address to the already full cq leading
> to undefined behavior.
>
> This commit restores the original order (cq reservation followed by skb
> allocation) and uses the xskq_prod_cancel helper to undo the cq reserve in
> event of skb allocation failure.

Thanks for fixing this Ciara.

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

> Fixes: 675716400da6 ("xdp: fix possible cq entry leak")
> Signed-off-by: Ciara Loftus <ciara.loftus@...el.com>
> ---
>  net/xdp/xsk.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 19ac872a6624..09002387987e 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -538,12 +538,6 @@ static int xsk_generic_xmit(struct sock *sk)
>                         goto out;
>                 }
>
> -               skb = xsk_build_skb(xs, &desc);
> -               if (IS_ERR(skb)) {
> -                       err = PTR_ERR(skb);
> -                       goto out;
> -               }
> -
>                 /* This is the backpressure mechanism for the Tx path.
>                  * Reserve space in the completion queue and only proceed
>                  * if there is space in it. This avoids having to implement
> @@ -552,11 +546,19 @@ static int xsk_generic_xmit(struct sock *sk)
>                 spin_lock_irqsave(&xs->pool->cq_lock, flags);
>                 if (xskq_prod_reserve(xs->pool->cq)) {
>                         spin_unlock_irqrestore(&xs->pool->cq_lock, flags);
> -                       kfree_skb(skb);
>                         goto out;
>                 }
>                 spin_unlock_irqrestore(&xs->pool->cq_lock, flags);
>
> +               skb = xsk_build_skb(xs, &desc);
> +               if (IS_ERR(skb)) {
> +                       err = PTR_ERR(skb);
> +                       spin_lock_irqsave(&xs->pool->cq_lock, flags);
> +                       xskq_prod_cancel(xs->pool->cq);
> +                       spin_unlock_irqrestore(&xs->pool->cq_lock, flags);
> +                       goto out;
> +               }
> +
>                 err = __dev_direct_xmit(skb, xs->queue_id);
>                 if  (err == NETDEV_TX_BUSY) {
>                         /* Tell user-space to retry the send */
> --
> 2.25.1
>