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] [day] [month] [year] [list]

Message-ID: <6ac071fd-ce47-b597-7436-0874f489bf07@iogearbox.net>
Date:   Tue, 2 Apr 2019 10:54:17 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Andrey Ignatov <rdna@...com>
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

On 04/02/2019 04:45 AM, Andrey Ignatov wrote:
> 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.

Ok, please make sure to also add a comment into retrieve_ptr_limit() on
why we don't handle var offset.

>> 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.

+1

> Thank you!
> 
>