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@xxxxxxxxx>
> 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@xxxxxxxxx>
>
> 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);
