On 04/02/2019 04:45 AM, Andrey Ignatov wrote:
> Daniel Borkmann <dan...@iogearbox.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 <r...@fb.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!
>
>
