RE: [PATCH bpf 1/2] bpf: fix an incorrect branch elimination by verifier

[John Fastabend <john.fastabend@gmail.com>]

Yonghong Song wrote:
> Wenbo reported an issue in [1] where a checking of null
> pointer is evaluated as always false. In this particular
> case, the program type is tp_btf and the pointer to
> compare is a PTR_TO_BTF_ID.
> 
> The current verifier considers PTR_TO_BTF_ID always
> reprents a non-null pointer, hence all PTR_TO_BTF_ID compares
> to 0 will be evaluated as always not-equal, which resulted
> in the branch elimination.
> 
> For example,
>  struct bpf_fentry_test_t {
>      struct bpf_fentry_test_t *a;
>  };
>  int BPF_PROG(test7, struct bpf_fentry_test_t *arg)
>  {
>      if (arg == 0)
>          test7_result = 1;
>      return 0;
>  }
>  int BPF_PROG(test8, struct bpf_fentry_test_t *arg)
>  {
>      if (arg->a == 0)
>          test8_result = 1;
>      return 0;
>  }
> 
> In above bpf programs, both branch arg == 0 and arg->a == 0
> are removed. This may not be what developer expected.
> 
> The bug is introduced by Commit cac616db39c2 ("bpf: Verifier
> track null pointer branch_taken with JNE and JEQ"),
> where PTR_TO_BTF_ID is considered to be non-null when evaluting
> pointer vs. scalar comparison. This may be added
> considering we have PTR_TO_BTF_ID_OR_NULL in the verifier
> as well.
> 
> PTR_TO_BTF_ID_OR_NULL is added to explicitly requires
> a non-NULL testing in selective cases. The current generic
> pointer tracing framework in verifier always
> assigns PTR_TO_BTF_ID so users does not need to
> check NULL pointer at every pointer level like a->b->c->d.

Thanks for fixing this.

But, don't we really need to check for null? I'm trying to
understand how we can avoid the check. If b is NULL above
we will have a problem no?

Also, we probably shouldn't name the type PTR_TO_BTF_ID if
it can be NULL. How about renaming it in bpf-next then although
it will be code churn... Or just fix the comments? Probably
bpf-next content though. wdyt? In my opinion the comments and
type names are really misleading as it stands.

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 3d2ade703a35..18051440f886 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -337,7 +337,7 @@ enum bpf_reg_type {
 	PTR_TO_TCP_SOCK_OR_NULL, /* reg points to struct tcp_sock or NULL */
 	PTR_TO_TP_BUFFER,	 /* reg points to a writable raw tp's buffer */
 	PTR_TO_XDP_SOCK,	 /* reg points to struct xdp_sock */
-	PTR_TO_BTF_ID,		 /* reg points to kernel struct */
+	PTR_TO_BTF_ID,		 /* reg points to kernel struct or NULL */
 	PTR_TO_BTF_ID_OR_NULL,	 /* reg points to kernel struct or NULL */
 	PTR_TO_MEM,		 /* reg points to valid memory region */
 	PTR_TO_MEM_OR_NULL,	 /* reg points to valid memory region or NULL */
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 7de98906ddf4..7412f9d2f0b5 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -500,7 +500,7 @@ static const char * const reg_type_str[] = {
 	[PTR_TO_TCP_SOCK_OR_NULL] = "tcp_sock_or_null",
 	[PTR_TO_TP_BUFFER]	= "tp_buffer",
 	[PTR_TO_XDP_SOCK]	= "xdp_sock",
-	[PTR_TO_BTF_ID]		= "ptr_",
+	[PTR_TO_BTF_ID]		= "ptr_or_null_",
 	[PTR_TO_BTF_ID_OR_NULL]	= "ptr_or_null_",
 	[PTR_TO_MEM]		= "mem",
 	[PTR_TO_MEM_OR_NULL]	= "mem_or_null",

> 
> We may not want to assign every PTR_TO_BTF_ID as
> PTR_TO_BTF_ID_OR_NULL as this will require a null test
> before pointer dereference which may cause inconvenience
> for developers. But we could avoid branch elimination
> to preserve original code intention.
> 
> This patch simply removed PTR_TO_BTD_ID from reg_type_not_null()
> in verifier, which prevented the above branches from being eliminated.
> 
>  [1]: https://lore.kernel.org/bpf/79dbb7c0-449d-83eb-5f4f-7af0cc269168@fb.com/T/
> 
> Fixes: cac616db39c2 ("bpf: Verifier track null pointer branch_taken with JNE and JEQ")
> Cc: Andrii Nakryiko <andriin@fb.com>
> Cc: John Fastabend <john.fastabend@gmail.com>
> Cc: Wenbo Zhang <ethercflow@gmail.com>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
>  kernel/bpf/verifier.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 8911d0576399..94cead5a43e5 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -399,8 +399,7 @@ static bool reg_type_not_null(enum bpf_reg_type type)
>  	return type == PTR_TO_SOCKET ||
>  		type == PTR_TO_TCP_SOCK ||
>  		type == PTR_TO_MAP_VALUE ||
> -		type == PTR_TO_SOCK_COMMON ||
> -	        type == PTR_TO_BTF_ID;
> +		type == PTR_TO_SOCK_COMMON;
>  }
>  
>  static bool reg_type_may_be_null(enum bpf_reg_type type)
> -- 
> 2.24.1
>