linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1] bpf: Set register type according to is_valid_access()
@ 2016-09-22 18:35 Mickaël Salaün
  2016-09-22 19:41 ` Daniel Borkmann
  0 siblings, 1 reply; 3+ messages in thread
From: Mickaël Salaün @ 2016-09-22 18:35 UTC (permalink / raw)
  To: linux-kernel
  Cc: Mickaël Salaün, Alexei Starovoitov, Andy Lutomirski,
	Daniel Borkmann, Kees Cook, Sargun Dhillon, Tejun Heo, netdev

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 <keescook@chromium.org>
Acked-by: Sargun Dhillon <sargun@sargun.me>
---
 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;
 		}
 
 	} else if (reg->type == FRAME_PTR || reg->type == PTR_TO_STACK) {
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH v1] bpf: Set register type according to is_valid_access()
  2016-09-22 18:35 [PATCH v1] bpf: Set register type according to is_valid_access() Mickaël Salaün
@ 2016-09-22 19:41 ` Daniel Borkmann
  2016-09-22 19:53   ` Mickaël Salaün
  0 siblings, 1 reply; 3+ messages in thread
From: Daniel Borkmann @ 2016-09-22 19:41 UTC (permalink / raw)
  To: Mickaël Salaün, linux-kernel
  Cc: Alexei Starovoitov, Andy Lutomirski, Kees Cook, Sargun Dhillon,
	Tejun Heo, netdev

On 09/22/2016 08:35 PM, Mickaël Salaün wrote:
> 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 <keescook@chromium.org>
> Acked-by: Sargun Dhillon <sargun@sargun.me>
> ---
>   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?

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

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH v1] bpf: Set register type according to is_valid_access()
  2016-09-22 19:41 ` Daniel Borkmann
@ 2016-09-22 19:53   ` Mickaël Salaün
  0 siblings, 0 replies; 3+ messages in thread
From: Mickaël Salaün @ 2016-09-22 19:53 UTC (permalink / raw)
  To: Daniel Borkmann, linux-kernel
  Cc: Alexei Starovoitov, Andy Lutomirski, Kees Cook, Sargun Dhillon,
	Tejun Heo, netdev


[-- Attachment #1.1: Type: text/plain, Size: 2680 bytes --]


On 22/09/2016 21:41, Daniel Borkmann wrote:
> On 09/22/2016 08:35 PM, Mickaël Salaün wrote:
>> 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 <keescook@chromium.org>
>> Acked-by: Sargun Dhillon <sargun@sargun.me>
>> ---
>>   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.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-09-22 19:53 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-22 18:35 [PATCH v1] bpf: Set register type according to is_valid_access() Mickaël Salaün
2016-09-22 19:41 ` Daniel Borkmann
2016-09-22 19:53   ` Mickaël Salaün

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).