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

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

[Mickaël Salaün]

This prevent future potential pointer leaks when an unprivileged eBPF
program will read a pointer value from its context. Even if
is_valid_access() returns a pointer type, the eBPF verifier replace it
with UNKNOWN_VALUE. The register value that contains a kernel address is
then allowed to leak. Moreover, this fix allows unprivileged eBPF
programs to use functions with (legitimate) pointer arguments.

Not an issue currently since reg_type is only set for PTR_TO_PACKET or
PTR_TO_PACKET_END in XDP and TC programs that can only be loaded as
privileged. For now, the only unprivileged eBPF program allowed is for
socket filtering and all the types from its context are UNKNOWN_VALUE.
However, this fix is important for future unprivileged eBPF programs
which could use pointers in their context.

Signed-off-by: Mickaël Salaün <mic@digikod.net>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
---
 kernel/bpf/verifier.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index daea765d72e6..adbc7c161ba5 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -795,9 +795,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;
 		}
 
 	} else if (reg->type == FRAME_PTR || reg->type == PTR_TO_STACK) {
-- 
2.9.3

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

[Daniel Borkmann]

On 09/24/2016 08:01 PM, Mickaël Salaün wrote:
> This prevent future potential pointer leaks when an unprivileged eBPF
> program will read a pointer value from its context. Even if
> is_valid_access() returns a pointer type, the eBPF verifier replace it
> with UNKNOWN_VALUE. The register value that contains a kernel address is
> then allowed to leak. Moreover, this fix allows unprivileged eBPF
> programs to use functions with (legitimate) pointer arguments.
>
> Not an issue currently since reg_type is only set for PTR_TO_PACKET or
> PTR_TO_PACKET_END in XDP and TC programs that can only be loaded as
> privileged. For now, the only unprivileged eBPF program allowed is for
> socket filtering and all the types from its context are UNKNOWN_VALUE.
> However, this fix is important for future unprivileged eBPF programs
> which could use pointers in their context.
>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>

Seems okay to me:

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

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

[Alexei Starovoitov]

On Mon, Sep 26, 2016 at 04:49:17PM +0200, Daniel Borkmann wrote:
> On 09/24/2016 08:01 PM, Mickaël Salaün wrote:
> >This prevent future potential pointer leaks when an unprivileged eBPF
> >program will read a pointer value from its context. Even if
> >is_valid_access() returns a pointer type, the eBPF verifier replace it
> >with UNKNOWN_VALUE. The register value that contains a kernel address is
> >then allowed to leak. Moreover, this fix allows unprivileged eBPF
> >programs to use functions with (legitimate) pointer arguments.
> >
> >Not an issue currently since reg_type is only set for PTR_TO_PACKET or
> >PTR_TO_PACKET_END in XDP and TC programs that can only be loaded as
> >privileged. For now, the only unprivileged eBPF program allowed is for
> >socket filtering and all the types from its context are UNKNOWN_VALUE.
> >However, this fix is important for future unprivileged eBPF programs
> >which could use pointers in their context.
> >
> >Signed-off-by: Mickaël Salaün <mic@digikod.net>
> >Cc: Alexei Starovoitov <ast@kernel.org>
> >Cc: Daniel Borkmann <daniel@iogearbox.net>
> 
> Seems okay to me:
> 
> Acked-by: Daniel Borkmann <daniel@iogearbox.net>

Acked-by: Alexei Starovoitov <ast@kernel.org>

Mickael, please mention [PATCH net-next] in subject next time.
Thanks

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

[David Miller]

From: Mickaël Salaün <mic@digikod.net>
Date: Sat, 24 Sep 2016 20:01:50 +0200

> This prevent future potential pointer leaks when an unprivileged eBPF
> program will read a pointer value from its context. Even if
> is_valid_access() returns a pointer type, the eBPF verifier replace it
> with UNKNOWN_VALUE. The register value that contains a kernel address is
> then allowed to leak. Moreover, this fix allows unprivileged eBPF
> programs to use functions with (legitimate) pointer arguments.
> 
> Not an issue currently since reg_type is only set for PTR_TO_PACKET or
> PTR_TO_PACKET_END in XDP and TC programs that can only be loaded as
> privileged. For now, the only unprivileged eBPF program allowed is for
> socket filtering and all the types from its context are UNKNOWN_VALUE.
> However, this fix is important for future unprivileged eBPF programs
> which could use pointers in their context.
> 
> Signed-off-by: Mickaël Salaün <mic@digikod.net>

Applied to net-next, thanks.