netdev - Re: [bpf PATCH v2 0/3] sockmap/ktls fixes

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: <20190425121244.20f88505@cakuba.netronome.com>
Date:   Thu, 25 Apr 2019 12:12:44 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     John Fastabend <john.fastabend@...il.com>
Cc:     ast@...nel.org, daniel@...earbox.net, netdev@...r.kernel.org,
        bpf@...r.kernel.org
Subject: Re: [bpf PATCH v2 0/3] sockmap/ktls fixes

On Thu, 25 Apr 2019 11:49:18 -0700, John Fastabend wrote:
> On 4/25/19 11:30 AM, Jakub Kicinski wrote:
> > On Thu, 25 Apr 2019 09:02:50 -0700, John Fastabend wrote:  
> >> Series of fixes for sockmap and ktls, see patches for descriptions.
> >>
> >> v2: fix build issue for CONFIG_TLS_DEVICE and fixup couple comments from
> >>     Jakub.  
> > 
> > Ah, right my comment about the rx side sleeping was fairly nonsensical,
> > the locking issues is that the work queue tries to lock the same socket.
> >   
> 
> Right.
> 
> > But I'm hitting some nasties, there is a UAF on a non-offload socket,
> > and offload dies fairly hard.  It _could_ be my offload patches on top,
> > but "they worked yesterday".  Digging deeper on the offload side,
> > here's the UAF:  
> 
> hmm OK I see what is happening. I could also only enable the unhash for
> SW/SW  base proto. So only with,
> 
>   prot[TLS_SW][TLS_SW].unhash
> 
> There is this on the offload side did I smash it somehow?
> 
>    prot[TLS_HW_RECORD][TLS_HW_RECORD].unhash       = tls_hw_unhash;


Um, I think you're good there, note that the TLS_HW_RECORD thing is not
the nice packet-based offload, it's the TOE stuff from Chelsio.  I'm
using TLS_HW.

> Also I have this in my stack,

Thanks, I will toss this in.

> commit 01628cbabdf2fbf0b710a399f54ae005d0963f3f (HEAD -> ktls-fixes,
> refs/patches/ktls-fixes/bpf-sockmap-only-stop-strp-if)
> Author: John Fastabend <john.fastabend@...il.com>
> Date:   Wed Apr 24 15:55:55 2019 -0700
> 
>     bpf: sockmap, only stop/flush strp if it was enabled at some point
> 
>     If we try to call strp_done on a parser that has never been
>     initialized, because the sockmap user is only using TX side for
>     example we get the following error.
> 
> 
>       [  883.422081] WARNING: CPU: 1 PID: 208 at kernel/workqueue.c:3030
> __flush_work+0x1ca/0x1e0
>       ...
>       [  883.422095] Workqueue: events sk_psock_destroy_deferred
>       [  883.422097] RIP: 0010:__flush_work+0x1ca/0x1e0
> 
> 
>     This had been wrapped in a 'if (psock->parser.enabled)' logic which
>     was broken because the strp_done() was never actually being called
>     because we do a strp_stop() earlier in the tear down logic will
>     set parser.enabled to false. This could result in a use after free
>     if work was still in the queue and was resolved by the patch here,
>     1d79895aef18f ("sk_msg: Always cancel strp work before freeing the
>     psock"). However, calling strp_stop(), done by the patch marked in
>     the fixes tag, only is useful if we never initialized a strp parser
>     program and never initialized the strp to start with. Because if
>     we had initialized a stream parser strp_stop() would have beencalled
>     by sk_psock_drop() earlier in the tear down process.  By forcing the
>     strp to stop we get past the WARNING in strp_done that checks
>     the stopped flag but calling cancel_work_sync on work that has never
>     been initialized is also wrong and generates the warning above.
> 
>     To fix check if the parser program exists. If the program exists
>     then the strp work has been initialized and must be sync'd and
>     cancelled before free'ing any structures. If no program exists we
>     never initialized the stream parser in the first place so skip the
>     sync/cancel logic implemented by strp_done.
> 
>     Finally, remove the strp_done its not needed and in the case where
>     we are using the
>     stream parser has already been called.
> 
>     Fixes: e8e3437762ad9 ("bpf: Stop the psock parser before canceling
> its work")
>     Signed-off-by: John Fastabend <john.fastabend@...il.com>
> 
> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> index 782ae9eb4dce..4b4b9ad4bb86 100644
> --- a/net/core/skmsg.c
> +++ b/net/core/skmsg.c
> @@ -555,8 +555,12 @@ static void sk_psock_destroy_deferred(struct
> work_struct *gc)
>         struct sk_psock *psock = container_of(gc, struct sk_psock, gc);
> 
>         /* No sk_callback_lock since already detached. */
> -       strp_stop(&psock->parser.strp);
> -       strp_done(&psock->parser.strp);
> +
> +       /* Parser has been stopped */
> +       if (psock->progs.skb_parser)
> +               strp_stop(&psock->parser.strp);
> +               strp_done(&psock->parser.strp);
> +       }
> 
>         cancel_work_sync(&psock->work);