[PATCH][next] bpf: fix swapped arguments in calls to check_buffer_access

[PATCH][next] bpf: fix swapped arguments in calls to check_buffer_access

[Colin King]

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>
---
 kernel/bpf/verifier.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

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);
-- 
2.27.0

Re: [PATCH][next] bpf: fix swapped arguments in calls to check_buffer_access

[Yonghong Song]

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>

Re: [PATCH][next] bpf: fix swapped arguments in calls to check_buffer_access

[Daniel Borkmann]

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

Re: [PATCH][next] bpf: fix swapped arguments in calls to check_buffer_access

[Colin Ian King]

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

Re: [PATCH][next] bpf: fix swapped arguments in calls to check_buffer_access

[Yonghong Song]

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