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