bpf: Check size for BTF-based ctx access of pointer members

Robert Morris reported the following program type which passes the
verifier in [0]:

SEC("struct_ops/bpf_cubic_init")
void BPF_PROG(bpf_cubic_init, struct sock *sk)
{
	asm volatile("r2 = *(u16*)(r1 + 0)");     // verifier should demand u64
	asm volatile("*(u32 *)(r2 +1504) = 0");   // 1280 in some configs
}

The second line may or may not work, but the first instruction shouldn't
pass, as it's a narrow load into the context structure of the struct ops
callback. The code falls back to btf_ctx_access to ensure correctness
and obtaining the types of pointers. Ensure that the size of the access
is correctly checked to be 8 bytes, otherwise the verifier thinks the
narrow load obtained a trusted BTF pointer and will permit loads/stores
as it sees fit.

Perform the check on size after we've verified that the load is for a
pointer field, as for scalar values narrow loads are fine. Access to
structs passed as arguments to a BPF program are also treated as
scalars, therefore no adjustment is needed in their case.

Existing verifier selftests are broken by this change, but because they
were incorrect. Verifier tests for d_path were performing narrow load
into context to obtain path pointer, had this program actually run it
would cause a crash. The same holds for verifier_btf_ctx_access tests.

  [0]: https://lore.kernel.org/bpf/51338.1732985814@localhost

Fixes: 9e15db66136a ("bpf: Implement accurate raw_tp context access via BTF")
Reported-by: Robert Morris <rtm@mit.edu>
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Link: https://lore.kernel.org/r/20241212092050.3204165-2-memxor@gmail.com
Signed-off-by: Alexei Starovoitov <ast@kernel.org>

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index e7a59e6462a9331d0acb17a88a4ebf641509c050..a63a03582f02d120f5e39d14b084d5cc922fd970 100644 (file)
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6543,6 +6543,12 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
                return false;
        }
 
+       if (size != sizeof(u64)) {
+               bpf_log(log, "func '%s' size %d must be 8\n",
+                       tname, size);
+               return false;
+       }
+
        /* check for PTR_TO_RDONLY_BUF_OR_NULL or PTR_TO_RDWR_BUF_OR_NULL */
        for (i = 0; i < prog->aux->ctx_arg_info_size; i++) {
                const struct bpf_ctx_arg_aux *ctx_arg_info = &prog->aux->ctx_arg_info[i];

diff --git a/tools/testing/selftests/bpf/progs/verifier_btf_ctx_access.c b/tools/testing/selftests/bpf/progs/verifier_btf_ctx_access.c
index a570e48b917accb6e4d08186f89a670c73b5b7f0..bfc3bf18fed4fefceec59bbbcc0a5c26207ab04c 100644 (file)
--- a/tools/testing/selftests/bpf/progs/verifier_btf_ctx_access.c
+++ b/tools/testing/selftests/bpf/progs/verifier_btf_ctx_access.c
@@ -11,7 +11,7 @@ __success __retval(0)
 __naked void btf_ctx_access_accept(void)
 {
        asm volatile ("                                 \
-       r2 = *(u32*)(r1 + 8);           /* load 2nd argument value (int pointer) */\
+       r2 = *(u64 *)(r1 + 8);          /* load 2nd argument value (int pointer) */\
        r0 = 0;                                         \
        exit;                                           \
 "      ::: __clobber_all);
@@ -23,7 +23,7 @@ __success __retval(0)
 __naked void ctx_access_u32_pointer_accept(void)
 {
        asm volatile ("                                 \
-       r2 = *(u32*)(r1 + 0);           /* load 1nd argument value (u32 pointer) */\
+       r2 = *(u64 *)(r1 + 0);          /* load 1nd argument value (u32 pointer) */\
        r0 = 0;                                         \
        exit;                                           \
 "      ::: __clobber_all);

diff --git a/tools/testing/selftests/bpf/progs/verifier_d_path.c b/tools/testing/selftests/bpf/progs/verifier_d_path.c
index ec79cbcfde91efe6df4f9aba6e92b280e07b681f..87e51a215558fda3c60e71cf99c18e9ebccfbaf5 100644 (file)
--- a/tools/testing/selftests/bpf/progs/verifier_d_path.c
+++ b/tools/testing/selftests/bpf/progs/verifier_d_path.c
@@ -11,7 +11,7 @@ __success __retval(0)
 __naked void d_path_accept(void)
 {
        asm volatile ("                                 \
-       r1 = *(u32*)(r1 + 0);                           \
+       r1 = *(u64 *)(r1 + 0);                          \
        r2 = r10;                                       \
        r2 += -8;                                       \
        r6 = 0;                                         \
@@ -31,7 +31,7 @@ __failure __msg("helper call is not allowed in probe")
 __naked void d_path_reject(void)
 {
        asm volatile ("                                 \
-       r1 = *(u32*)(r1 + 0);                           \
+       r1 = *(u64 *)(r1 + 0);                          \
        r2 = r10;                                       \
        r2 += -8;                                       \
        r6 = 0;                                         \