netdev - Re: [PATCH bpf-next 0/2] bpf: Support variable offset stack access from helpers

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: <20190402024459.GA36969@rdna-mbp.dhcp.thefacebook.com>
Date:   Tue, 2 Apr 2019 02:45:02 +0000
From:   Andrey Ignatov <rdna@...com>
To:     Daniel Borkmann <daniel@...earbox.net>
CC:     Alexei Starovoitov <ast@...com>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Network Development <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH bpf-next 0/2] bpf: Support variable offset stack access
 from helpers

Daniel Borkmann <daniel@...earbox.net> [Mon, 2019-04-01 11:58 -0700]:
> On 04/01/2019 07:23 PM, Alexei Starovoitov wrote:
> > On 4/1/19 9:09 AM, Daniel Borkmann wrote:
> >> On 03/29/2019 08:10 PM, Alexei Starovoitov wrote:
> >>> On Thu, Mar 28, 2019 at 6:02 PM Andrey Ignatov <rdna@...com> wrote:
> >>>>
> >>>> The patch set adds support for stack access with variable offset from helpers.
> >>>>
> >>>> Patch 1 is the main patch in the set and provides more details.
> >>>> Patch 2 adds selftests for new functionality.
> >>>
> >>> Applied. Thanks
> >>
> >> Hmm, I think this series needs more work unfortunately. The selftests are only
> >> checking root-only programs, which is way to little. For !root we do the spectre
> >> masking for map and stack ALU, and that hasn't been adapted here, so it will
> >> generate a wrong masking for runtime since it doesn't take variable part into
> >> account. Andrey, please take a look.
> > 
> > right. may be we should allow this for root only then?

Thanks Daniel! I missed this spectre masking for stack ALU.

I read the code and see that, yeah, retrieve_ptr_limit, that is called
from sanitize_ptr_alu, doesn't take variable offset into account.

Though since sanitation happens only for unpriv mode I agree with Alexei
that we can just deny variable offsets for unpriv. That's probably the
simplest option and it should be fine for use-case I have for variable
offset (bpf_strto{l,ul}).

I'll send follow-up with this change.

> Probably yeah, though thinking more about it, what about the case where we pass
> in raw (uninitialized) buffers from stack into a helper? Our assumption has
> been thus far that given the size is const, we can mark them in verifier as
> initialized after the call (as helpers memset it on error). With variable access
> it could be within a given range from verification side, but at runtime it's
> concrete value, meaning, upon function return we could leak uninitialized stack
> where verifier thinks it has been initialized by the helper. I think the set
> doesn't address this either, unfortunately. (So would need to be restricted to
> helpers where we pass always initialized buffers into it.)

Thanks again for another great catch! I'll change it so that if (meta &&
meta->raw_mode) (i.e. buffer wasn't initialized), variable offset will
be rejected.

I'll also add more tests for both scenarios and send follow-up with all
these changes.

Thank you!


-- 
Andrey Ignatov