diff mbox series

[bpf-next,v3,2/2] bpf: improve error logging for zero-sized accesses

Message ID 20231220170604.183380-3-andreimatei1@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series bpf: Simplify checking size of helper accesses | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-VM_Test-44 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-43 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-46 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-45 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-47 success Logs for x86_64-llvm-18 / veristat
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next
netdev/ynl success SINGLE THREAD; Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 1127 this patch: 1128
netdev/cc_maintainers warning 14 maintainers not CCed: sdf@google.com haoluo@google.com andrii@kernel.org kpsingh@kernel.org martin.lau@linux.dev jolsa@kernel.org daniel@iogearbox.net shuah@kernel.org yonghong.song@linux.dev linux-kselftest@vger.kernel.org ast@kernel.org mykolal@fb.com song@kernel.org john.fastabend@gmail.com
netdev/build_clang fail Errors and warnings before: 1143 this patch: 1146
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 1154 this patch: 1155
netdev/checkpatch warning CHECK: Alignment should match open parenthesis CHECK: Please don't use multiple blank lines CHECK: spaces preferred around that '-' (ctx:VxV) WARNING: line length of 104 exceeds 80 columns WARNING: line length of 108 exceeds 80 columns WARNING: line length of 113 exceeds 80 columns WARNING: line length of 118 exceeds 80 columns WARNING: line length of 120 exceeds 80 columns WARNING: line length of 125 exceeds 80 columns WARNING: line length of 129 exceeds 80 columns WARNING: line length of 138 exceeds 80 columns WARNING: line length of 81 exceeds 80 columns WARNING: line length of 84 exceeds 80 columns WARNING: line length of 87 exceeds 80 columns WARNING: line length of 95 exceeds 80 columns WARNING: quoted string split across lines
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc

Commit Message

Andrei Matei Dec. 20, 2023, 5:06 p.m. UTC 
This patch improves the verifier error messages for illegal zero-sized
memory accesses. The previous patch made these messages focus on the
register containing the size of the access, instead of focusing on the
register containing the dereferenced pointer. This was more correct, but
removed useful information about the pointer. This patch brings this
information back and then some. We now have complete error messages that
are also consistent across pointer types.

Signed-off-by: Andrei Matei <andreimatei1@gmail.com>
---
 kernel/bpf/verifier.c                         | 63 ++++++++++++++++++-
 .../bpf/progs/verifier_helper_value_access.c  | 10 +--
 .../selftests/bpf/progs/verifier_raw_stack.c  |  4 +-
 3 files changed, 69 insertions(+), 8 deletions(-)

Comments

Andrii Nakryiko Dec. 21, 2023, 4:51 a.m. UTC | #1 
On Wed, Dec 20, 2023 at 9:06 AM Andrei Matei <andreimatei1@gmail.com> wrote:
>
> This patch improves the verifier error messages for illegal zero-sized
> memory accesses. The previous patch made these messages focus on the
> register containing the size of the access, instead of focusing on the
> register containing the dereferenced pointer. This was more correct, but
> removed useful information about the pointer. This patch brings this
> information back and then some. We now have complete error messages that
> are also consistent across pointer types.
>
> Signed-off-by: Andrei Matei <andreimatei1@gmail.com>
> ---
>  kernel/bpf/verifier.c                         | 63 ++++++++++++++++++-
>  .../bpf/progs/verifier_helper_value_access.c  | 10 +--
>  .../selftests/bpf/progs/verifier_raw_stack.c  |  4 +-
>  3 files changed, 69 insertions(+), 8 deletions(-)
>

I left a bunch of nitpicky comments. TBH, I now feel that the simple
message you added in the previous patch is probably adequate for the
error it is notifying about. Sorry for making you work on adding more
and now pushing back against it. But if others like the improved
message, then I don't feel strongly enough to argue against :)

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 4409b8f2b0f3..6f333c5c47f8 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -7256,6 +7256,67 @@ static int check_helper_mem_access(struct bpf_verifier_env *env, int regno,
>         }
>  }
>
> +/* Helper function for logging an error about an invalid attempt to perform a
> + * (possibly) zero-sized memory access. The pointer being dereferenced is in
> + * register @ptr_regno, and the size of the access is in register @size_regno.
> + * The size register is assumed to either be a constant zero or have a zero lower
> + * bound.
> + *
> + * Logs a message like:
> + * invalid zero-size read. Size comes from R2=0. Attempting to dereference *map_value R1: off=[0,4] value_size=48
> + */
> +static void log_zero_size_access_err(struct bpf_verifier_env *env,
> +                             int ptr_regno,
> +                             int size_regno)
> +{
> +       struct bpf_reg_state *ptr_reg = &cur_regs(env)[ptr_regno];
> +       struct bpf_reg_state *size_reg = &cur_regs(env)[size_regno];
> +       const bool size_is_const = tnum_is_const(size_reg->var_off);
> +       const char *ptr_type_str = reg_type_str(env, ptr_reg->type);
> +       /* allocate a few buffers to be used as parts of the error message */
> +       char size_range_buf[32] = {0}, max_size_buf[32] = {0}, off_buf[64] = {0};
> +       s64 min_off, max_off;
> +
> +       if (!size_is_const) {
> +               snprintf(size_range_buf, sizeof(size_range_buf),
> +                       "[0,%lld]", size_reg->umax_value);
> +       }
> +
> +       if (tnum_is_const(ptr_reg->var_off)) {
> +               min_off = (s64)ptr_reg->var_off.value + ptr_reg->off;
> +               snprintf(off_buf, sizeof(off_buf), "%lld", min_off);
> +       } else {
> +               min_off = ptr_reg->smin_value + ptr_reg->off;
> +               max_off = ptr_reg->smax_value + ptr_reg->off;
> +               snprintf(off_buf, sizeof(off_buf), "[%lld,%lld]", min_off, max_off);
> +       }
> +

if var_off is const, smin==smax (unless some unrelated bug). so I'd
just normalize this entire if/else to always be printing [%lld,%lld]
<- [smin+off, smax+off]. It's a bit more verbose for const cases, but
it feels acceptable for me.

> +       /* attempt to figure out info about the maximum offset that could be allowed */
> +       switch (ptr_reg->type) {
> +       case PTR_TO_MAP_KEY:
> +               snprintf(max_size_buf, sizeof(max_size_buf), "key_size=%d", ptr_reg->map_ptr->key_size);
> +               break;
> +       case PTR_TO_MAP_VALUE:
> +               snprintf(max_size_buf, sizeof(max_size_buf), "value_size=%d", ptr_reg->map_ptr->value_size);
> +               break;
> +       case PTR_TO_PACKET:
> +       case PTR_TO_PACKET_META:
> +               snprintf(max_size_buf, sizeof(max_size_buf), "packet_size=%d", ptr_reg->range);
> +               break;
> +       case PTR_TO_MEM:
> +               snprintf(max_size_buf, sizeof(max_size_buf), "mem_size=%d", ptr_reg->mem_size);

break missing

> +       default:
> +               snprintf(max_size_buf, sizeof(max_size_buf), "max_size=N/A");
> +       }

again, subjective, but I feel like just calculating relevant memory
region size as int and then emitting generic `mem_size=%d` is probably
absolutely sufficient here (see example message below) and we won't
need to use yet another buffer for formatting.

> +
> +       verbose(env, "invalid %szero-size read. Size comes from R%d=%s. "
> +               "Attempting to dereference *%s R%d: off=%s %s\n",

It's very subjective, and if others like it I won't insist, but the
style of this multi-sentence message just doesn't fit with the rest of
the verifier log format. Something along the lines of:

"invalid possible zero-size read of R2 bytes from R1 mem_size=%d"

it's simple and provides all the necessary information (what's the
actual R2 range is not relevant to this message, the important is that
zero size is possible; user should see R2 state few lines earlier in
the log where it's assigned anyways; same for R1 mem_size would help a
bit to identify what that thing is (e.g., which map it belongs to),
but again, R1 full state will be somewhere very close by, so maybe
even that is not necessary)

> +               size_is_const ? "" : "possibly ",
> +               size_regno, size_is_const ? "0" : size_range_buf,
> +               ptr_type_str, ptr_regno, off_buf, max_size_buf);
> +}
> +
> +
>  /* verify arguments to helpers or kfuncs consisting of a pointer and an access
>   * size.
>   *
> @@ -7298,7 +7359,7 @@ static int check_mem_size_reg(struct bpf_verifier_env *env,
>         }
>
>         if (reg->umin_value == 0 && !zero_size_allowed) {
> -               verbose(env, "R%d invalid zero-sized read\n", regno);
> +               log_zero_size_access_err(env, regno-1, regno);

code style: regno - 1


>                 return -EACCES;
>         }
>

[...]

pw-bot: cr
diff mbox series

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 4409b8f2b0f3..6f333c5c47f8 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -7256,6 +7256,67 @@  static int check_helper_mem_access(struct bpf_verifier_env *env, int regno,
 	}
 }
 
+/* Helper function for logging an error about an invalid attempt to perform a
+ * (possibly) zero-sized memory access. The pointer being dereferenced is in
+ * register @ptr_regno, and the size of the access is in register @size_regno.
+ * The size register is assumed to either be a constant zero or have a zero lower
+ * bound.
+ *
+ * Logs a message like:
+ * invalid zero-size read. Size comes from R2=0. Attempting to dereference *map_value R1: off=[0,4] value_size=48
+ */
+static void log_zero_size_access_err(struct bpf_verifier_env *env,
+			      int ptr_regno,
+			      int size_regno)
+{
+	struct bpf_reg_state *ptr_reg = &cur_regs(env)[ptr_regno];
+	struct bpf_reg_state *size_reg = &cur_regs(env)[size_regno];
+	const bool size_is_const = tnum_is_const(size_reg->var_off);
+	const char *ptr_type_str = reg_type_str(env, ptr_reg->type);
+	/* allocate a few buffers to be used as parts of the error message */
+	char size_range_buf[32] = {0}, max_size_buf[32] = {0}, off_buf[64] = {0};
+	s64 min_off, max_off;
+
+	if (!size_is_const) {
+		snprintf(size_range_buf, sizeof(size_range_buf),
+			"[0,%lld]", size_reg->umax_value);
+	}
+
+	if (tnum_is_const(ptr_reg->var_off)) {
+		min_off = (s64)ptr_reg->var_off.value + ptr_reg->off;
+		snprintf(off_buf, sizeof(off_buf), "%lld", min_off);
+	} else {
+		min_off = ptr_reg->smin_value + ptr_reg->off;
+		max_off = ptr_reg->smax_value + ptr_reg->off;
+		snprintf(off_buf, sizeof(off_buf), "[%lld,%lld]", min_off, max_off);
+	}
+
+	/* attempt to figure out info about the maximum offset that could be allowed */
+	switch (ptr_reg->type) {
+	case PTR_TO_MAP_KEY:
+		snprintf(max_size_buf, sizeof(max_size_buf), "key_size=%d", ptr_reg->map_ptr->key_size);
+		break;
+	case PTR_TO_MAP_VALUE:
+		snprintf(max_size_buf, sizeof(max_size_buf), "value_size=%d", ptr_reg->map_ptr->value_size);
+		break;
+	case PTR_TO_PACKET:
+	case PTR_TO_PACKET_META:
+		snprintf(max_size_buf, sizeof(max_size_buf), "packet_size=%d", ptr_reg->range);
+		break;
+	case PTR_TO_MEM:
+		snprintf(max_size_buf, sizeof(max_size_buf), "mem_size=%d", ptr_reg->mem_size);
+	default:
+		snprintf(max_size_buf, sizeof(max_size_buf), "max_size=N/A");
+	}
+
+	verbose(env, "invalid %szero-size read. Size comes from R%d=%s. "
+		"Attempting to dereference *%s R%d: off=%s %s\n",
+		size_is_const ? "" : "possibly ",
+		size_regno, size_is_const ? "0" : size_range_buf,
+		ptr_type_str, ptr_regno, off_buf, max_size_buf);
+}
+
+
 /* verify arguments to helpers or kfuncs consisting of a pointer and an access
  * size.
  *
@@ -7298,7 +7359,7 @@  static int check_mem_size_reg(struct bpf_verifier_env *env,
 	}
 
 	if (reg->umin_value == 0 && !zero_size_allowed) {
-		verbose(env, "R%d invalid zero-sized read\n", regno);
+		log_zero_size_access_err(env, regno-1, regno);
 		return -EACCES;
 	}
 
diff --git a/tools/testing/selftests/bpf/progs/verifier_helper_value_access.c b/tools/testing/selftests/bpf/progs/verifier_helper_value_access.c
index 137cce939711..9fe10f63c931 100644
--- a/tools/testing/selftests/bpf/progs/verifier_helper_value_access.c
+++ b/tools/testing/selftests/bpf/progs/verifier_helper_value_access.c
@@ -96,7 +96,7 @@  l0_%=:	exit;						\
  */
 SEC("tracepoint")
 __description("helper access to map: empty range")
-__failure __msg("R2 invalid zero-sized read")
+__failure __msg("invalid zero-size read. Size comes from R2=0. Attempting to dereference *map_value R1: off=0 value_size=48")
 __naked void access_to_map_empty_range(void)
 {
 	asm volatile ("					\
@@ -123,7 +123,7 @@  l0_%=:	exit;						\
  */
 SEC("tracepoint")
 __description("helper access to map: possibly-empty range")
-__failure __msg("R2 invalid zero-sized read")
+__failure __msg("invalid possibly zero-size read. Size comes from R2=[0,4]. Attempting to dereference *map_value R1: off=0 value_size=48")
 __naked void access_to_map_possibly_empty_range(void)
 {
 	asm volatile ("                                         \
@@ -258,7 +258,7 @@  l0_%=:	exit;						\
 
 SEC("tracepoint")
 __description("helper access to adjusted map (via const imm): empty range")
-__failure __msg("R2 invalid zero-sized read")
+__failure __msg("invalid zero-size read. Size comes from R2=0. Attempting to dereference *map_value R1: off=4 value_size=48")
 __naked void via_const_imm_empty_range(void)
 {
 	asm volatile ("					\
@@ -423,7 +423,7 @@  l0_%=:	exit;						\
 
 SEC("tracepoint")
 __description("helper access to adjusted map (via const reg): empty range")
-__failure __msg("R2 invalid zero-sized read")
+__failure __msg("invalid zero-size read. Size comes from R2=0. Attempting to dereference *map_value R1: off=0 value_size=48")
 __naked void via_const_reg_empty_range(void)
 {
 	asm volatile ("					\
@@ -593,7 +593,7 @@  l0_%=:	exit;						\
 
 SEC("tracepoint")
 __description("helper access to adjusted map (via variable): empty range")
-__failure __msg("R2 invalid zero-sized read")
+__failure __msg("invalid zero-size read. Size comes from R2=0. Attempting to dereference *map_value R1: off=[0,4] value_size=48")
 __naked void map_via_variable_empty_range(void)
 {
 	asm volatile ("					\
diff --git a/tools/testing/selftests/bpf/progs/verifier_raw_stack.c b/tools/testing/selftests/bpf/progs/verifier_raw_stack.c
index 3dbda85e2997..c133d9d2c45e 100644
--- a/tools/testing/selftests/bpf/progs/verifier_raw_stack.c
+++ b/tools/testing/selftests/bpf/progs/verifier_raw_stack.c
@@ -64,7 +64,7 @@  __naked void load_bytes_negative_len_2(void)
 
 SEC("tc")
 __description("raw_stack: skb_load_bytes, zero len")
-__failure __msg("R4 invalid zero-sized read")
+__failure __msg("invalid zero-size read. Size comes from R4=0. Attempting to dereference *fp R3: off=-8 max_size=N/A")
 __naked void skb_load_bytes_zero_len(void)
 {
 	asm volatile ("					\
@@ -333,7 +333,7 @@  __naked void load_bytes_invalid_access_5(void)
 
 SEC("tc")
 __description("raw_stack: skb_load_bytes, invalid access 6")
-__failure __msg("invalid zero-sized read")
+__failure __msg("invalid zero-size read. Size comes from R4=0. Attempting to dereference *fp R3: off=-512 max_size=N/A")
 __naked void load_bytes_invalid_access_6(void)
 {
 	asm volatile ("					\