| Message ID | 158507155667.15666.4189866174878249746.stgit@john-Precision-5820-Tower |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | ALU32 bounds tracking support | expand |
On Tue, Mar 24, 2020 at 10:39:16AM -0700, John Fastabend wrote: > Mark 32-bit subreg region with max value because do_refine_retval_range() > catches functions with int return type (We will assume here that int is > a 32-bit type). Marking 64-bit region could be dangerous if upper bits > are not zero which could be possible. > > Two reasons to pull this out of original patch. First it makes the original > fix impossible to backport. And second I've not seen this as being problematic > in practice unlike the other case. > > Fixes: 849fa50662fbc ("bpf/verifier: refine retval R0 state for bpf_get_stack helper") > Signed-off-by: John Fastabend <john.fastabend@gmail.com> > --- > kernel/bpf/verifier.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 6372fa4..3731109 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -4328,7 +4328,7 @@ static void do_refine_retval_range(struct bpf_reg_state *regs, int ret_type, > func_id != BPF_FUNC_probe_read_str)) > return; > > - ret_reg->smax_value = meta->msize_max_value; > + ret_reg->s32_max_value = meta->msize_max_value; I think this is not correct. These two special helpers are invoked via BPF_CALL_x() which has u64 return value. So despite having 'int' return in bpf_helper_defs.h the upper 32-bit will be correct. I think this patch should do: ret_reg->smax_value = meta->msize_max_value; ret_reg->s32_max_value = meta->msize_max_value;
Alexei Starovoitov wrote: > On Tue, Mar 24, 2020 at 10:39:16AM -0700, John Fastabend wrote: > > Mark 32-bit subreg region with max value because do_refine_retval_range() > > catches functions with int return type (We will assume here that int is > > a 32-bit type). Marking 64-bit region could be dangerous if upper bits > > are not zero which could be possible. > > > > Two reasons to pull this out of original patch. First it makes the original > > fix impossible to backport. And second I've not seen this as being problematic > > in practice unlike the other case. > > > > Fixes: 849fa50662fbc ("bpf/verifier: refine retval R0 state for bpf_get_stack helper") > > Signed-off-by: John Fastabend <john.fastabend@gmail.com> > > --- > > kernel/bpf/verifier.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index 6372fa4..3731109 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -4328,7 +4328,7 @@ static void do_refine_retval_range(struct bpf_reg_state *regs, int ret_type, > > func_id != BPF_FUNC_probe_read_str)) > > return; > > > > - ret_reg->smax_value = meta->msize_max_value; > > + ret_reg->s32_max_value = meta->msize_max_value; > > I think this is not correct. > These two special helpers are invoked via BPF_CALL_x() which has u64 return value. > So despite having 'int' return in bpf_helper_defs.h the upper 32-bit will be correct. > I think this patch should do: > ret_reg->smax_value = meta->msize_max_value; > ret_reg->s32_max_value = meta->msize_max_value; OK, I missed the u64 in BPF_CALL_x(). Setting both smax and s32_max looks correct. My logic above is wrong so I'll fix that. Thanks.
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 6372fa4..3731109 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -4328,7 +4328,7 @@ static void do_refine_retval_range(struct bpf_reg_state *regs, int ret_type, func_id != BPF_FUNC_probe_read_str)) return; - ret_reg->smax_value = meta->msize_max_value; + ret_reg->s32_max_value = meta->msize_max_value; __reg_deduce_bounds(ret_reg); __reg_bound_offset(ret_reg); __update_reg_bounds(ret_reg);
Mark 32-bit subreg region with max value because do_refine_retval_range() catches functions with int return type (We will assume here that int is a 32-bit type). Marking 64-bit region could be dangerous if upper bits are not zero which could be possible. Two reasons to pull this out of original patch. First it makes the original fix impossible to backport. And second I've not seen this as being problematic in practice unlike the other case. Fixes: 849fa50662fbc ("bpf/verifier: refine retval R0 state for bpf_get_stack helper") Signed-off-by: John Fastabend <john.fastabend@gmail.com> --- kernel/bpf/verifier.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)