| Message ID | 20200727175411.155179-1-colin.king@canonical.com |
|---|---|
| State | Accepted |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [next] bpf: fix swapped arguments in calls to check_buffer_access | expand |
On 7/27/20 10:54 AM, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > There are a couple of arguments of the boolean flag zero_size_allowed > and the char pointer buf_info when calling to function check_buffer_access > that are swapped by mistake. Fix these by swapping them to correct > the argument ordering. > > Addresses-Coverity: ("Array compared to 0") > Fixes: afbf21dce668 ("bpf: Support readonly/readwrite buffers in verifier") > Signed-off-by: Colin Ian King <colin.king@canonical.com> Thanks for the fix! Acked-by: Yonghong Song <yhs@fb.com>
On 7/27/20 11:39 PM, Yonghong Song wrote: > On 7/27/20 10:54 AM, Colin King wrote: >> From: Colin Ian King <colin.king@canonical.com> >> >> There are a couple of arguments of the boolean flag zero_size_allowed >> and the char pointer buf_info when calling to function check_buffer_access >> that are swapped by mistake. Fix these by swapping them to correct >> the argument ordering. >> >> Addresses-Coverity: ("Array compared to 0") >> Fixes: afbf21dce668 ("bpf: Support readonly/readwrite buffers in verifier") >> Signed-off-by: Colin Ian King <colin.king@canonical.com> > > Thanks for the fix! > Acked-by: Yonghong Song <yhs@fb.com> Sigh, thanks for the fix Colin, applied! Yonghong, could you follow-up with BPF selftest test cases that exercise these paths? Thx
On 28/07/2020 11:43, Daniel Borkmann wrote: > On 7/27/20 11:39 PM, Yonghong Song wrote: >> On 7/27/20 10:54 AM, Colin King wrote: >>> From: Colin Ian King <colin.king@canonical.com> >>> >>> There are a couple of arguments of the boolean flag zero_size_allowed >>> and the char pointer buf_info when calling to function >>> check_buffer_access >>> that are swapped by mistake. Fix these by swapping them to correct >>> the argument ordering. >>> >>> Addresses-Coverity: ("Array compared to 0") >>> Fixes: afbf21dce668 ("bpf: Support readonly/readwrite buffers in >>> verifier") >>> Signed-off-by: Colin Ian King <colin.king@canonical.com> >> >> Thanks for the fix! >> Acked-by: Yonghong Song <yhs@fb.com> > > Sigh, thanks for the fix Colin, applied! Yonghong, could you follow-up with > BPF selftest test cases that exercise these paths? Thx No problem. Thanks to static analysis, it catches a lot of subtle bugs like this. Colin
On 7/28/20 3:43 AM, Daniel Borkmann wrote: > On 7/27/20 11:39 PM, Yonghong Song wrote: >> On 7/27/20 10:54 AM, Colin King wrote: >>> From: Colin Ian King <colin.king@canonical.com> >>> >>> There are a couple of arguments of the boolean flag zero_size_allowed >>> and the char pointer buf_info when calling to function >>> check_buffer_access >>> that are swapped by mistake. Fix these by swapping them to correct >>> the argument ordering. >>> >>> Addresses-Coverity: ("Array compared to 0") >>> Fixes: afbf21dce668 ("bpf: Support readonly/readwrite buffers in >>> verifier") >>> Signed-off-by: Colin Ian King <colin.king@canonical.com> >> >> Thanks for the fix! >> Acked-by: Yonghong Song <yhs@fb.com> > > Sigh, thanks for the fix Colin, applied! Yonghong, could you follow-up with > BPF selftest test cases that exercise these paths? Thx This will be triggered with a verifier rejection path, e.g., negative offset from the base. I will send a follow-up patch soon. BTW, using llvm to build the kernel (without this change), the compiler actually issues a warning: -bash-4.4$ make -j100 LLVM=1 && make LLVM=1 vmlinux GEN Makefile ... CC kernel/bpf/verifier.o /data/users/yhs/work/net-next/kernel/bpf/verifier.c:3481:18: warning: expression which evaluates to zero treate$ as a null pointer constant of type 'const char *' [-Wnon-literal-null-conversion] "rdonly", false, ^~~~~ /data/users/yhs/work/net-next/kernel/bpf/verifier.c:3487:16: warning: expression which evaluates to zero treate$ as a null pointer constant of type 'const char *' [-Wnon-literal-null-conversion] "rdwr", false, ^~~~~ 2 warnings generated. AR kernel/bpf/built-in.a Looks like I need to use LLVM compiler more often...
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index cd14e70f2d07..88bb25d08bf8 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -3477,14 +3477,14 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn regno, reg_type_str[reg->type]); return -EACCES; } - err = check_buffer_access(env, reg, regno, off, size, "rdonly", - false, + err = check_buffer_access(env, reg, regno, off, size, false, + "rdonly", &env->prog->aux->max_rdonly_access); if (!err && value_regno >= 0) mark_reg_unknown(env, regs, value_regno); } else if (reg->type == PTR_TO_RDWR_BUF) { - err = check_buffer_access(env, reg, regno, off, size, "rdwr", - false, + err = check_buffer_access(env, reg, regno, off, size, false, + "rdwr", &env->prog->aux->max_rdwr_access); if (!err && t == BPF_READ && value_regno >= 0) mark_reg_unknown(env, regs, value_regno);