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@xxxxxx/T/
>
> Fixes: cac616db39c2 ("bpf: Verifier track null pointer branch_taken with JNE and JEQ")
> Cc: Andrii Nakryiko <andriin@xxxxxx>
> Cc: John Fastabend <john.fastabend@xxxxxxxxx>
> Cc: Wenbo Zhang <ethercflow@xxxxxxxxx>
> Signed-off-by: Yonghong Song <yhs@xxxxxx>
> ---
> 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
>
