Re: [PATCH v1] bpf: Set register type according to is_valid_access()

On 22/09/2016 21:41, Daniel Borkmann wrote:

On 09/22/2016 08:35 PM, Mickaël Salaün wrote:

quoted

This fix a pointer leak when an unprivileged eBPF program read a pointer
value from the context. Even if is_valid_access() returns a pointer
type, the eBPF verifier replace it with UNKNOWN_VALUE. The register
value containing an address is then allowed to leak. Moreover, this
prevented unprivileged eBPF programs to use functions with (legitimate)
pointer arguments.

This bug is not an issue for now because the only unprivileged eBPF
program allowed is of type BPF_PROG_TYPE_SOCKET_FILTER and all the types
from its context are UNKNOWN_VALUE. However, this fix is important for
future unprivileged eBPF program types which could use pointers in their
context.

Signed-off-by: Mickaël Salaün <mic@digikod.net>
Fixes: 969bf05eb3ce ("bpf: direct packet access")
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Kees Cook <redacted>
Acked-by: Sargun Dhillon <redacted>
---
  kernel/bpf/verifier.c | 6 ++----
  1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index daea765d72e6..0698ccd67715 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c

@@ -794,10 +794,8 @@ static int check_mem_access(struct verifier_env

*env, u32 regno, int off,
          }
          err = check_ctx_access(env, off, size, t, &reg_type);
          if (!err && t == BPF_READ && value_regno >= 0) {
-            mark_reg_unknown_value(state->regs, value_regno);
-            if (env->allow_ptr_leaks)
-                /* note that reg.[id|off|range] == 0 */
-                state->regs[value_regno].type = reg_type;
+            /* note that reg.[id|off|range] == 0 */
+            state->regs[value_regno].type = reg_type;

True that it's not an issue currently, since reg_type is only set for
PTR_TO_PACKET/PTR_TO_PACKET_END in xdp and tc programs that can only be
loaded as privileged. So not an issue for BPF_PROG_TYPE_SOCKET_FILTER.

One thing I don't quite follow is why you remove the
mark_reg_unknown_value()
as this also clears imm? I think this could result in an actual verifier
bug when it would reuse previous tracked imm value of that dst register?

Good catch, I missed the imm initialization. I'm going to send a new patch.

quoted

          }

      } else if (reg->type == FRAME_PTR || reg->type == PTR_TO_STACK) {