netdev - Re: [bpf PATCH v2 0/3] Sockmap RCU splat fix

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: <CAADnVQLJo6PFBm8USM1gbAxFMTE297XbDtvhuFTUYPdJG9WpaA@mail.gmail.com>
Date:   Sun, 28 Jun 2020 08:40:41 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     John Fastabend <john.fastabend@...il.com>
Cc:     Martin KaFai Lau <kafai@...com>,
        Jakub Sitnicki <jakub@...udflare.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Network Development <netdev@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>
Subject: Re: [bpf PATCH v2 0/3] Sockmap RCU splat fix

On Thu, Jun 25, 2020 at 4:12 PM John Fastabend <john.fastabend@...il.com> wrote:
>
> Fix a splat introduced by recent changes to avoid skipping ingress policy
> when kTLS is enabled. The RCU splat was introduced because in the non-TLS
> case the caller is wrapped in an rcu_read_lock/unlock. But, in the TLS
> case we have a reference to the psock and the caller did not wrap its
> call in rcu_read_lock/unlock.
>
> To fix extend the RCU section to include the redirect case which was
> missed. From v1->v2 I changed the location a bit to simplify the code
> some. See patch 1.
>
> But, then Martin asked why it was not needed in the non-TLS case. The
> answer for patch 1 was, as stated above, because the caller has the
> rcu read lock. However, there was still a missing case where a BPF
> user could in-theory line up a set of parameters to hit a case
> where the code was entered from strparser side from a different context
> then the initial caller. To hit this user would need a parser program
> to return value greater than skb->len then an ENOMEM error could happen
> in the strparser codepath triggering strparser to retry from a workqueue
> and without rcu_read_lock original caller used. See patch 2 for details.
>
> Finally, we don't actually have any selftests for parser returning a
> value geater than skb->len so add one in patch 3. This is especially
> needed because at least I don't have any code that uses the parser
> to return value greater than skb->len. So I wouldn't have caught any
> errors here in my own testing.
>
> Thanks, John
>
> v1->v2: simplify code in patch 1 some and add patches 2 and 3.

Applied. Thanks