| Message ID | ac90247f2ef390966fd4629cfc1bf1c41eaed789.1554236245.git.rdna@fb.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: Fix indirect var_off stack access support | expand |
On 04/02/2019 10:19 PM, Andrey Ignatov wrote: > It's hard to guarantee that whole memory is marked as initialized on > helper return if uninitialized stack is accessed with variable offset > since specific bounds are unknown to verifier. This may cause > uninitialized stack leaking. > > Reject such an access in check_stack_boundary to prevent possible > leaking. > > There are no known use-cases for indirect uninitialized stack access > with variable offset so it shouldn't break anything. > > Fixes: 2011fccfb61b ("bpf: Support variable offset stack access from helpers") > Reported-by: Daniel Borkmann <daniel@iogearbox.net> > Signed-off-by: Andrey Ignatov <rdna@fb.com> > --- > kernel/bpf/verifier.c | 25 +++++++++++++++++++------ > 1 file changed, 19 insertions(+), 6 deletions(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index b7a7a9caa82f..12b84307ffa8 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -2212,7 +2212,26 @@ static int check_stack_boundary(struct bpf_verifier_env *env, int regno, > zero_size_allowed); > if (err) > return err; > + if (meta && meta->raw_mode) { > + meta->access_size = access_size; > + meta->regno = regno; > + return 0; > + } > } else { > + /* Only initialized buffer on stack is allowed to be accessed > + * with variable offset. With uninitialized buffer it's hard to > + * guarantee that whole memory is marked as initialized on > + * helper return since specific bounds are unknown what may > + * cause uninitialized stack leaking. > + */ > + if (meta && meta->raw_mode) { > + char tn_buf[48]; > + > + tnum_strn(tn_buf, sizeof(tn_buf), reg->var_off); > + verbose(env, "R%d invalid indirect access to uninitialized stack var_off=%s\n", > + regno, tn_buf); > + return -EACCES; > + } Hmm, I think we should probably handle this in similar way like we do in case of variable stack access when it comes to stack size: if (!tnum_is_const(reg->var_off)) /* For unprivileged variable accesses, disable raw * mode so that the program is required to * initialize all the memory that the helper could * just partially fill up. */ meta = NULL; So we error out naturally on the loop later where we also mark for liveness, and also allow for more flexibility if we know stack must already be initialized in this range. > min_off = reg->smin_value + reg->off; > max_off = reg->umax_value + reg->off; > err = __check_stack_boundary(env, regno, min_off, access_size, > @@ -2225,12 +2244,6 @@ static int check_stack_boundary(struct bpf_verifier_env *env, int regno, > return err; > } > > - if (meta && meta->raw_mode) { > - meta->access_size = access_size; > - meta->regno = regno; > - return 0; > - } This can then also stay as-is. > for (i = min_off; i < max_off + access_size; i++) { > u8 *stype; > >
On 04/03/2019 06:21 PM, Daniel Borkmann wrote: > On 04/02/2019 10:19 PM, Andrey Ignatov wrote: >> It's hard to guarantee that whole memory is marked as initialized on >> helper return if uninitialized stack is accessed with variable offset >> since specific bounds are unknown to verifier. This may cause >> uninitialized stack leaking. >> >> Reject such an access in check_stack_boundary to prevent possible >> leaking. >> >> There are no known use-cases for indirect uninitialized stack access >> with variable offset so it shouldn't break anything. >> >> Fixes: 2011fccfb61b ("bpf: Support variable offset stack access from helpers") >> Reported-by: Daniel Borkmann <daniel@iogearbox.net> >> Signed-off-by: Andrey Ignatov <rdna@fb.com> >> --- >> kernel/bpf/verifier.c | 25 +++++++++++++++++++------ >> 1 file changed, 19 insertions(+), 6 deletions(-) >> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >> index b7a7a9caa82f..12b84307ffa8 100644 >> --- a/kernel/bpf/verifier.c >> +++ b/kernel/bpf/verifier.c >> @@ -2212,7 +2212,26 @@ static int check_stack_boundary(struct bpf_verifier_env *env, int regno, >> zero_size_allowed); >> if (err) >> return err; >> + if (meta && meta->raw_mode) { >> + meta->access_size = access_size; >> + meta->regno = regno; >> + return 0; >> + } >> } else { >> + /* Only initialized buffer on stack is allowed to be accessed >> + * with variable offset. With uninitialized buffer it's hard to >> + * guarantee that whole memory is marked as initialized on >> + * helper return since specific bounds are unknown what may >> + * cause uninitialized stack leaking. >> + */ >> + if (meta && meta->raw_mode) { >> + char tn_buf[48]; >> + >> + tnum_strn(tn_buf, sizeof(tn_buf), reg->var_off); >> + verbose(env, "R%d invalid indirect access to uninitialized stack var_off=%s\n", >> + regno, tn_buf); >> + return -EACCES; >> + } > > Hmm, I think we should probably handle this in similar way like we do > in case of variable stack access when it comes to stack size: > > if (!tnum_is_const(reg->var_off)) > /* For unprivileged variable accesses, disable raw > * mode so that the program is required to > * initialize all the memory that the helper could > * just partially fill up. > */ > meta = NULL; > > So we error out naturally on the loop later where we also mark for > liveness, and also allow for more flexibility if we know stack must > already be initialized in this range. > >> min_off = reg->smin_value + reg->off; >> max_off = reg->umax_value + reg->off; >> err = __check_stack_boundary(env, regno, min_off, access_size, Btw, shouldn't above two additions be sanity checked for wrap-around resp. truncation? >> @@ -2225,12 +2244,6 @@ static int check_stack_boundary(struct bpf_verifier_env *env, int regno, >> return err; >> } >> >> - if (meta && meta->raw_mode) { >> - meta->access_size = access_size; >> - meta->regno = regno; >> - return 0; >> - } > > This can then also stay as-is. > >> for (i = min_off; i < max_off + access_size; i++) { >> u8 *stype; >> >> >
Daniel Borkmann <daniel@iogearbox.net> [Wed, 2019-04-03 09:22 -0700]: > On 04/02/2019 10:19 PM, Andrey Ignatov wrote: > > It's hard to guarantee that whole memory is marked as initialized on > > helper return if uninitialized stack is accessed with variable offset > > since specific bounds are unknown to verifier. This may cause > > uninitialized stack leaking. > > > > Reject such an access in check_stack_boundary to prevent possible > > leaking. > > > > There are no known use-cases for indirect uninitialized stack access > > with variable offset so it shouldn't break anything. > > > > Fixes: 2011fccfb61b ("bpf: Support variable offset stack access from helpers") > > Reported-by: Daniel Borkmann <daniel@iogearbox.net> > > Signed-off-by: Andrey Ignatov <rdna@fb.com> > > --- > > kernel/bpf/verifier.c | 25 +++++++++++++++++++------ > > 1 file changed, 19 insertions(+), 6 deletions(-) > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index b7a7a9caa82f..12b84307ffa8 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -2212,7 +2212,26 @@ static int check_stack_boundary(struct bpf_verifier_env *env, int regno, > > zero_size_allowed); > > if (err) > > return err; > > + if (meta && meta->raw_mode) { > > + meta->access_size = access_size; > > + meta->regno = regno; > > + return 0; > > + } > > } else { > > + /* Only initialized buffer on stack is allowed to be accessed > > + * with variable offset. With uninitialized buffer it's hard to > > + * guarantee that whole memory is marked as initialized on > > + * helper return since specific bounds are unknown what may > > + * cause uninitialized stack leaking. > > + */ > > + if (meta && meta->raw_mode) { > > + char tn_buf[48]; > > + > > + tnum_strn(tn_buf, sizeof(tn_buf), reg->var_off); > > + verbose(env, "R%d invalid indirect access to uninitialized stack var_off=%s\n", > > + regno, tn_buf); > > + return -EACCES; > > + } > > Hmm, I think we should probably handle this in similar way like we do > in case of variable stack access when it comes to stack size: > > if (!tnum_is_const(reg->var_off)) > /* For unprivileged variable accesses, disable raw > * mode so that the program is required to > * initialize all the memory that the helper could > * just partially fill up. > */ > meta = NULL; > > So we error out naturally on the loop later where we also mark for > liveness, and also allow for more flexibility if we know stack must > already be initialized in this range. Yeah, I think this will work. This will change the logic a bit though. E.g. logic in this patch will deny variable offset stack access to ARG_PTR_TO_UNINIT_MEM no matter if corresponding stack memory is initialized or not. But with `meta = NULL` verifier will accept access to ARG_PTR_TO_UNINIT_MEM on stack if that part of the stack is fully initialized for all possible offsets. I think the latter should be fine since if all possible bytes that can be accessed are already initialized then there should not be problem on return from the helper. I'll switch to `meta = NULL` in v3. Though given the difference in the logic, let me know if you prefer to keep the one in this patch. Thanks. > > min_off = reg->smin_value + reg->off; > > max_off = reg->umax_value + reg->off; > > err = __check_stack_boundary(env, regno, min_off, access_size, > > @@ -2225,12 +2244,6 @@ static int check_stack_boundary(struct bpf_verifier_env *env, int regno, > > return err; > > } > > > > - if (meta && meta->raw_mode) { > > - meta->access_size = access_size; > > - meta->regno = regno; > > - return 0; > > - } > > This can then also stay as-is. > > > for (i = min_off; i < max_off + access_size; i++) { > > u8 *stype; > > > > >
Daniel Borkmann <daniel@iogearbox.net> [Wed, 2019-04-03 09:46 -0700]: > On 04/03/2019 06:21 PM, Daniel Borkmann wrote: > > On 04/02/2019 10:19 PM, Andrey Ignatov wrote: > >> It's hard to guarantee that whole memory is marked as initialized on > >> helper return if uninitialized stack is accessed with variable offset > >> since specific bounds are unknown to verifier. This may cause > >> uninitialized stack leaking. > >> > >> Reject such an access in check_stack_boundary to prevent possible > >> leaking. > >> > >> There are no known use-cases for indirect uninitialized stack access > >> with variable offset so it shouldn't break anything. > >> > >> Fixes: 2011fccfb61b ("bpf: Support variable offset stack access from helpers") > >> Reported-by: Daniel Borkmann <daniel@iogearbox.net> > >> Signed-off-by: Andrey Ignatov <rdna@fb.com> > >> --- > >> kernel/bpf/verifier.c | 25 +++++++++++++++++++------ > >> 1 file changed, 19 insertions(+), 6 deletions(-) > >> > >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > >> index b7a7a9caa82f..12b84307ffa8 100644 > >> --- a/kernel/bpf/verifier.c > >> +++ b/kernel/bpf/verifier.c > >> @@ -2212,7 +2212,26 @@ static int check_stack_boundary(struct bpf_verifier_env *env, int regno, > >> zero_size_allowed); > >> if (err) > >> return err; > >> + if (meta && meta->raw_mode) { > >> + meta->access_size = access_size; > >> + meta->regno = regno; > >> + return 0; > >> + } > >> } else { > >> + /* Only initialized buffer on stack is allowed to be accessed > >> + * with variable offset. With uninitialized buffer it's hard to > >> + * guarantee that whole memory is marked as initialized on > >> + * helper return since specific bounds are unknown what may > >> + * cause uninitialized stack leaking. > >> + */ > >> + if (meta && meta->raw_mode) { > >> + char tn_buf[48]; > >> + > >> + tnum_strn(tn_buf, sizeof(tn_buf), reg->var_off); > >> + verbose(env, "R%d invalid indirect access to uninitialized stack var_off=%s\n", > >> + regno, tn_buf); > >> + return -EACCES; > >> + } > > > > Hmm, I think we should probably handle this in similar way like we do > > in case of variable stack access when it comes to stack size: > > > > if (!tnum_is_const(reg->var_off)) > > /* For unprivileged variable accesses, disable raw > > * mode so that the program is required to > > * initialize all the memory that the helper could > > * just partially fill up. > > */ > > meta = NULL; > > > > So we error out naturally on the loop later where we also mark for > > liveness, and also allow for more flexibility if we know stack must > > already be initialized in this range. > > > >> min_off = reg->smin_value + reg->off; > >> max_off = reg->umax_value + reg->off; > >> err = __check_stack_boundary(env, regno, min_off, access_size, > > Btw, shouldn't above two additions be sanity checked for wrap-around > resp. truncation? Good question. As I can see, both reg->smin_value and reg->off are checked by check_reg_sane_offset() in adjust_ptr_min_max_vals() that handles pointer arithmetics. And I don't know how to come up with variable offset w/o pointer arithmetics, i.e. these both should be in (-BPF_MAX_VAR_OFF; BPF_MAX_VAR_OFF). As for reg->umax_value, I see that it's checked in check_func_arg() before calling to check_helper_mem_access() (that in turn calls to check_stack_boundary()): if (reg->umax_value >= BPF_MAX_VAR_SIZ) { verbose(env, "R%d unbounded memory access, use 'var &= const' or 'if (var < const)'\n", regno); return -EACCES; } So my understanding is with all these checks that happen beforehand, there should not be overflow and int is used for offset in both the old code, that handles constant offset, and this new code for variable offset. Let me know if I'm missing something. > >> @@ -2225,12 +2244,6 @@ static int check_stack_boundary(struct bpf_verifier_env *env, int regno, > >> return err; > >> } > >> > >> - if (meta && meta->raw_mode) { > >> - meta->access_size = access_size; > >> - meta->regno = regno; > >> - return 0; > >> - } > > > > This can then also stay as-is. > > > >> for (i = min_off; i < max_off + access_size; i++) { > >> u8 *stype; > >> > >> > > >
On 04/03/2019 11:02 PM, Andrey Ignatov wrote: > Daniel Borkmann <daniel@iogearbox.net> [Wed, 2019-04-03 09:22 -0700]: >> On 04/02/2019 10:19 PM, Andrey Ignatov wrote: >>> It's hard to guarantee that whole memory is marked as initialized on >>> helper return if uninitialized stack is accessed with variable offset >>> since specific bounds are unknown to verifier. This may cause >>> uninitialized stack leaking. >>> >>> Reject such an access in check_stack_boundary to prevent possible >>> leaking. >>> >>> There are no known use-cases for indirect uninitialized stack access >>> with variable offset so it shouldn't break anything. >>> >>> Fixes: 2011fccfb61b ("bpf: Support variable offset stack access from helpers") >>> Reported-by: Daniel Borkmann <daniel@iogearbox.net> >>> Signed-off-by: Andrey Ignatov <rdna@fb.com> >>> --- >>> kernel/bpf/verifier.c | 25 +++++++++++++++++++------ >>> 1 file changed, 19 insertions(+), 6 deletions(-) >>> >>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >>> index b7a7a9caa82f..12b84307ffa8 100644 >>> --- a/kernel/bpf/verifier.c >>> +++ b/kernel/bpf/verifier.c >>> @@ -2212,7 +2212,26 @@ static int check_stack_boundary(struct bpf_verifier_env *env, int regno, >>> zero_size_allowed); >>> if (err) >>> return err; >>> + if (meta && meta->raw_mode) { >>> + meta->access_size = access_size; >>> + meta->regno = regno; >>> + return 0; >>> + } >>> } else { >>> + /* Only initialized buffer on stack is allowed to be accessed >>> + * with variable offset. With uninitialized buffer it's hard to >>> + * guarantee that whole memory is marked as initialized on >>> + * helper return since specific bounds are unknown what may >>> + * cause uninitialized stack leaking. >>> + */ >>> + if (meta && meta->raw_mode) { >>> + char tn_buf[48]; >>> + >>> + tnum_strn(tn_buf, sizeof(tn_buf), reg->var_off); >>> + verbose(env, "R%d invalid indirect access to uninitialized stack var_off=%s\n", >>> + regno, tn_buf); >>> + return -EACCES; >>> + } >> >> Hmm, I think we should probably handle this in similar way like we do >> in case of variable stack access when it comes to stack size: >> >> if (!tnum_is_const(reg->var_off)) >> /* For unprivileged variable accesses, disable raw >> * mode so that the program is required to >> * initialize all the memory that the helper could >> * just partially fill up. >> */ >> meta = NULL; >> >> So we error out naturally on the loop later where we also mark for >> liveness, and also allow for more flexibility if we know stack must >> already be initialized in this range. > > Yeah, I think this will work. > > This will change the logic a bit though. > > E.g. logic in this patch will deny variable offset stack access to > ARG_PTR_TO_UNINIT_MEM no matter if corresponding stack memory is > initialized or not. > > But with `meta = NULL` verifier will accept access to > ARG_PTR_TO_UNINIT_MEM on stack if that part of the stack is fully > initialized for all possible offsets. > > I think the latter should be fine since if all possible bytes that can > be accessed are already initialized then there should not be problem on > return from the helper. > > I'll switch to `meta = NULL` in v3. Though given the difference in the > logic, let me know if you prefer to keep the one in this patch. Thanks. Yes I know, I mentioned it in my email wrt more flexibility, but probably not communicated clear enough. I think that's totally fine. >>> min_off = reg->smin_value + reg->off; >>> max_off = reg->umax_value + reg->off; >>> err = __check_stack_boundary(env, regno, min_off, access_size, >>> @@ -2225,12 +2244,6 @@ static int check_stack_boundary(struct bpf_verifier_env *env, int regno, >>> return err; >>> } >>> >>> - if (meta && meta->raw_mode) { >>> - meta->access_size = access_size; >>> - meta->regno = regno; >>> - return 0; >>> - } >> >> This can then also stay as-is. >> >>> for (i = min_off; i < max_off + access_size; i++) { >>> u8 *stype; >>> >>> >> >
On 04/03/2019 11:57 PM, Andrey Ignatov wrote: > Daniel Borkmann <daniel@iogearbox.net> [Wed, 2019-04-03 09:46 -0700]: >> On 04/03/2019 06:21 PM, Daniel Borkmann wrote: >>> On 04/02/2019 10:19 PM, Andrey Ignatov wrote: >>>> It's hard to guarantee that whole memory is marked as initialized on >>>> helper return if uninitialized stack is accessed with variable offset >>>> since specific bounds are unknown to verifier. This may cause >>>> uninitialized stack leaking. >>>> >>>> Reject such an access in check_stack_boundary to prevent possible >>>> leaking. >>>> >>>> There are no known use-cases for indirect uninitialized stack access >>>> with variable offset so it shouldn't break anything. >>>> >>>> Fixes: 2011fccfb61b ("bpf: Support variable offset stack access from helpers") >>>> Reported-by: Daniel Borkmann <daniel@iogearbox.net> >>>> Signed-off-by: Andrey Ignatov <rdna@fb.com> >>>> --- >>>> kernel/bpf/verifier.c | 25 +++++++++++++++++++------ >>>> 1 file changed, 19 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >>>> index b7a7a9caa82f..12b84307ffa8 100644 >>>> --- a/kernel/bpf/verifier.c >>>> +++ b/kernel/bpf/verifier.c >>>> @@ -2212,7 +2212,26 @@ static int check_stack_boundary(struct bpf_verifier_env *env, int regno, >>>> zero_size_allowed); >>>> if (err) >>>> return err; >>>> + if (meta && meta->raw_mode) { >>>> + meta->access_size = access_size; >>>> + meta->regno = regno; >>>> + return 0; >>>> + } >>>> } else { >>>> + /* Only initialized buffer on stack is allowed to be accessed >>>> + * with variable offset. With uninitialized buffer it's hard to >>>> + * guarantee that whole memory is marked as initialized on >>>> + * helper return since specific bounds are unknown what may >>>> + * cause uninitialized stack leaking. >>>> + */ >>>> + if (meta && meta->raw_mode) { >>>> + char tn_buf[48]; >>>> + >>>> + tnum_strn(tn_buf, sizeof(tn_buf), reg->var_off); >>>> + verbose(env, "R%d invalid indirect access to uninitialized stack var_off=%s\n", >>>> + regno, tn_buf); >>>> + return -EACCES; >>>> + } >>> >>> Hmm, I think we should probably handle this in similar way like we do >>> in case of variable stack access when it comes to stack size: >>> >>> if (!tnum_is_const(reg->var_off)) >>> /* For unprivileged variable accesses, disable raw >>> * mode so that the program is required to >>> * initialize all the memory that the helper could >>> * just partially fill up. >>> */ >>> meta = NULL; >>> >>> So we error out naturally on the loop later where we also mark for >>> liveness, and also allow for more flexibility if we know stack must >>> already be initialized in this range. >>> >>>> min_off = reg->smin_value + reg->off; >>>> max_off = reg->umax_value + reg->off; >>>> err = __check_stack_boundary(env, regno, min_off, access_size, >> >> Btw, shouldn't above two additions be sanity checked for wrap-around >> resp. truncation? > > Good question. > > As I can see, both reg->smin_value and reg->off are checked by > check_reg_sane_offset() in adjust_ptr_min_max_vals() that handles > pointer arithmetics. And I don't know how to come up with variable > offset w/o pointer arithmetics, i.e. these both should be in > (-BPF_MAX_VAR_OFF; BPF_MAX_VAR_OFF). > > As for reg->umax_value, I see that it's checked in check_func_arg() > before calling to check_helper_mem_access() (that in turn calls to > check_stack_boundary()): > > if (reg->umax_value >= BPF_MAX_VAR_SIZ) { > verbose(env, "R%d unbounded memory access, use 'var &= const' or 'if (var < const)'\n", > regno); > return -EACCES; > } > > So my understanding is with all these checks that happen beforehand, > there should not be overflow and int is used for offset in both the old > code, that handles constant offset, and this new code for variable > offset. The latter one is on the reg with size argument, not on the reg with pointer to stack. check_helper_mem_access() calls 'regno - 1' for the one where the register holds the pointer to stack value.
Daniel Borkmann <daniel@iogearbox.net> [Wed, 2019-04-03 16:19 -0700]: > On 04/03/2019 11:57 PM, Andrey Ignatov wrote: > > Daniel Borkmann <daniel@iogearbox.net> [Wed, 2019-04-03 09:46 -0700]: > >> On 04/03/2019 06:21 PM, Daniel Borkmann wrote: > >>> On 04/02/2019 10:19 PM, Andrey Ignatov wrote: ... > >>> > >>>> min_off = reg->smin_value + reg->off; > >>>> max_off = reg->umax_value + reg->off; > >>>> err = __check_stack_boundary(env, regno, min_off, access_size, > >> > >> Btw, shouldn't above two additions be sanity checked for wrap-around > >> resp. truncation? > > > > Good question. > > > > As I can see, both reg->smin_value and reg->off are checked by > > check_reg_sane_offset() in adjust_ptr_min_max_vals() that handles > > pointer arithmetics. And I don't know how to come up with variable > > offset w/o pointer arithmetics, i.e. these both should be in > > (-BPF_MAX_VAR_OFF; BPF_MAX_VAR_OFF). > > > > As for reg->umax_value, I see that it's checked in check_func_arg() > > before calling to check_helper_mem_access() (that in turn calls to > > check_stack_boundary()): > > > > if (reg->umax_value >= BPF_MAX_VAR_SIZ) { > > verbose(env, "R%d unbounded memory access, use 'var &= const' or 'if (var < const)'\n", > > regno); > > return -EACCES; > > } > > > > So my understanding is with all these checks that happen beforehand, > > there should not be overflow and int is used for offset in both the old > > code, that handles constant offset, and this new code for variable > > offset. > > The latter one is on the reg with size argument, not on the reg with pointer > to stack. check_helper_mem_access() calls 'regno - 1' for the one where the > register holds the pointer to stack value. You're right of course. I saw how size argument is handled and that 'regno - 1', but for some reason missed it while writing previous answer. I was able to write a program that exploits max_off overflow, so yeah, it is a problem. I'll fix it and send v3. Thanks for catching all these tricky things!
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index b7a7a9caa82f..12b84307ffa8 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -2212,7 +2212,26 @@ static int check_stack_boundary(struct bpf_verifier_env *env, int regno, zero_size_allowed); if (err) return err; + if (meta && meta->raw_mode) { + meta->access_size = access_size; + meta->regno = regno; + return 0; + } } else { + /* Only initialized buffer on stack is allowed to be accessed + * with variable offset. With uninitialized buffer it's hard to + * guarantee that whole memory is marked as initialized on + * helper return since specific bounds are unknown what may + * cause uninitialized stack leaking. + */ + if (meta && meta->raw_mode) { + char tn_buf[48]; + + tnum_strn(tn_buf, sizeof(tn_buf), reg->var_off); + verbose(env, "R%d invalid indirect access to uninitialized stack var_off=%s\n", + regno, tn_buf); + return -EACCES; + } min_off = reg->smin_value + reg->off; max_off = reg->umax_value + reg->off; err = __check_stack_boundary(env, regno, min_off, access_size, @@ -2225,12 +2244,6 @@ static int check_stack_boundary(struct bpf_verifier_env *env, int regno, return err; } - if (meta && meta->raw_mode) { - meta->access_size = access_size; - meta->regno = regno; - return 0; - } - for (i = min_off; i < max_off + access_size; i++) { u8 *stype;
It's hard to guarantee that whole memory is marked as initialized on helper return if uninitialized stack is accessed with variable offset since specific bounds are unknown to verifier. This may cause uninitialized stack leaking. Reject such an access in check_stack_boundary to prevent possible leaking. There are no known use-cases for indirect uninitialized stack access with variable offset so it shouldn't break anything. Fixes: 2011fccfb61b ("bpf: Support variable offset stack access from helpers") Reported-by: Daniel Borkmann <daniel@iogearbox.net> Signed-off-by: Andrey Ignatov <rdna@fb.com> --- kernel/bpf/verifier.c | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-)