On 6/30/20 10:51 AM, John Fastabend wrote:
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?
It depends with particular data structure.
If users are sure once pointer 'a' is valid and a->b, a->b->c, a->b->c
are all valid pointers, user may just write a->b->c->d. this happens
to some bcc scripts. So non-null pointer is checked.
But if user thinks a->b->c is null, he may write
type *p = a->b->c;
if (p)
p->d;
Or user just takes advantage of kernel bpf guarded exception handling
and do a->b->c->d even if a->b->c could be null.
if the result is 0, it means a->b->c is null or major fault,
otherwise it is not 0.
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.
So PTR_TO_BTF_ID actually means it may be null but not checking
is enforced and pointer tracing is always allowed.
PTR_TO_BTF_ID_OR_NULL means it may be null and checking against
NULL is needed to allow further pointer tracing.
To avoid code churn, we can add these comments in bpf-next.
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
