| Message ID | cover.1553821057.git.rdna@fb.com |
|---|---|
| Headers | show |
| Series | bpf: Support variable offset stack access from helpers | expand |
On Thu, Mar 28, 2019 at 6:02 PM Andrey Ignatov <rdna@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
On 03/29/2019 08:10 PM, Alexei Starovoitov wrote: > On Thu, Mar 28, 2019 at 6:02 PM Andrey Ignatov <rdna@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.
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@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?
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@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? 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, Daniel
Daniel Borkmann <daniel@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 <rdna@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!
On 04/02/2019 04:45 AM, Andrey Ignatov wrote: > Daniel Borkmann <daniel@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 <rdna@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! > >