* [PATCH bpf] bpf: fix sanitation rewrite in case of non-pointers
@ 2019-03-01 21:05 Daniel Borkmann
2019-03-01 23:18 ` Song Liu
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Daniel Borkmann @ 2019-03-01 21:05 UTC (permalink / raw)
To: ast; +Cc: afabre, marek, bpf, netdev, Daniel Borkmann
Marek reported that he saw an issue with the below snippet in that
timing measurements where off when loaded as unpriv while results
were reasonable when loaded as privileged:
[...]
uint64_t a = bpf_ktime_get_ns();
uint64_t b = bpf_ktime_get_ns();
uint64_t delta = b - a;
if ((int64_t)delta > 0) {
[...]
Turns out there is a bug where a corner case is missing in the fix
d3bd7413e0ca ("bpf: fix sanitation of alu op with pointer / scalar
type from different paths"), namely fixup_bpf_calls() only checks
whether aux has a non-zero alu_state, but it also needs to test for
the case of BPF_ALU_NON_POINTER since in both occasions we need to
skip the masking rewrite (as there is nothing to mask).
Fixes: d3bd7413e0ca ("bpf: fix sanitation of alu op with pointer / scalar type from different paths")
Reported-by: Marek Majkowski <marek@cloudflare.com>
Reported-by: Arthur Fabre <afabre@cloudflare.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Link: https://lore.kernel.org/netdev/CAJPywTJqP34cK20iLM5YmUMz9KXQOdu1-+BZrGMAGgLuBWz7fg@mail.gmail.com/T/
---
[ Test case will be routed via bpf-next to avoid useless merge churn
due to test_verifier rework in bpf-next. ]
kernel/bpf/verifier.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 8f295b790297..5fcce2f4209d 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -6920,7 +6920,8 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
u32 off_reg;
aux = &env->insn_aux_data[i + delta];
- if (!aux->alu_state)
+ if (!aux->alu_state ||
+ aux->alu_state == BPF_ALU_NON_POINTER)
continue;
isneg = aux->alu_state & BPF_ALU_NEG_VALUE;
--
2.17.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH bpf] bpf: fix sanitation rewrite in case of non-pointers
2019-03-01 21:05 [PATCH bpf] bpf: fix sanitation rewrite in case of non-pointers Daniel Borkmann
@ 2019-03-01 23:18 ` Song Liu
2019-03-01 23:22 ` Daniel Borkmann
2019-03-02 5:30 ` Alexei Starovoitov
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Song Liu @ 2019-03-01 23:18 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Alexei Starovoitov, afabre, Marek Majkowski, bpf, Networking
On Fri, Mar 1, 2019 at 1:06 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> Marek reported that he saw an issue with the below snippet in that
> timing measurements where off when loaded as unpriv while results
> were reasonable when loaded as privileged:
>
> [...]
> uint64_t a = bpf_ktime_get_ns();
> uint64_t b = bpf_ktime_get_ns();
> uint64_t delta = b - a;
> if ((int64_t)delta > 0) {
> [...]
>
> Turns out there is a bug where a corner case is missing in the fix
> d3bd7413e0ca ("bpf: fix sanitation of alu op with pointer / scalar
> type from different paths"), namely fixup_bpf_calls() only checks
> whether aux has a non-zero alu_state, but it also needs to test for
> the case of BPF_ALU_NON_POINTER since in both occasions we need to
> skip the masking rewrite (as there is nothing to mask).
>
> Fixes: d3bd7413e0ca ("bpf: fix sanitation of alu op with pointer / scalar type from different paths")
> Reported-by: Marek Majkowski <marek@cloudflare.com>
> Reported-by: Arthur Fabre <afabre@cloudflare.com>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Link: https://lore.kernel.org/netdev/CAJPywTJqP34cK20iLM5YmUMz9KXQOdu1-+BZrGMAGgLuBWz7fg@mail.gmail.com/T/
> ---
> [ Test case will be routed via bpf-next to avoid useless merge churn
> due to test_verifier rework in bpf-next. ]
>
> kernel/bpf/verifier.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 8f295b790297..5fcce2f4209d 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -6920,7 +6920,8 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
> u32 off_reg;
>
> aux = &env->insn_aux_data[i + delta];
> - if (!aux->alu_state)
> + if (!aux->alu_state ||
> + aux->alu_state == BPF_ALU_NON_POINTER)
alu_state is a bitmap. Shall we check "aux->alu_state &
BPF_ALU_NON_POINTER" here?
Thanks,
Song
> continue;
>
> isneg = aux->alu_state & BPF_ALU_NEG_VALUE;
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf] bpf: fix sanitation rewrite in case of non-pointers
2019-03-01 23:18 ` Song Liu
@ 2019-03-01 23:22 ` Daniel Borkmann
2019-03-01 23:42 ` Song Liu
0 siblings, 1 reply; 9+ messages in thread
From: Daniel Borkmann @ 2019-03-01 23:22 UTC (permalink / raw)
To: Song Liu; +Cc: Alexei Starovoitov, afabre, Marek Majkowski, bpf, Networking
On 03/02/2019 12:18 AM, Song Liu wrote:
> On Fri, Mar 1, 2019 at 1:06 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>
>> Marek reported that he saw an issue with the below snippet in that
>> timing measurements where off when loaded as unpriv while results
>> were reasonable when loaded as privileged:
>>
>> [...]
>> uint64_t a = bpf_ktime_get_ns();
>> uint64_t b = bpf_ktime_get_ns();
>> uint64_t delta = b - a;
>> if ((int64_t)delta > 0) {
>> [...]
>>
>> Turns out there is a bug where a corner case is missing in the fix
>> d3bd7413e0ca ("bpf: fix sanitation of alu op with pointer / scalar
>> type from different paths"), namely fixup_bpf_calls() only checks
>> whether aux has a non-zero alu_state, but it also needs to test for
>> the case of BPF_ALU_NON_POINTER since in both occasions we need to
>> skip the masking rewrite (as there is nothing to mask).
>>
>> Fixes: d3bd7413e0ca ("bpf: fix sanitation of alu op with pointer / scalar type from different paths")
>> Reported-by: Marek Majkowski <marek@cloudflare.com>
>> Reported-by: Arthur Fabre <afabre@cloudflare.com>
>> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
>> Link: https://lore.kernel.org/netdev/CAJPywTJqP34cK20iLM5YmUMz9KXQOdu1-+BZrGMAGgLuBWz7fg@mail.gmail.com/T/
>> ---
>> [ Test case will be routed via bpf-next to avoid useless merge churn
>> due to test_verifier rework in bpf-next. ]
>>
>> kernel/bpf/verifier.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 8f295b790297..5fcce2f4209d 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -6920,7 +6920,8 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
>> u32 off_reg;
>>
>> aux = &env->insn_aux_data[i + delta];
>> - if (!aux->alu_state)
>> + if (!aux->alu_state ||
>> + aux->alu_state == BPF_ALU_NON_POINTER)
>
> alu_state is a bitmap. Shall we check "aux->alu_state &
> BPF_ALU_NON_POINTER" here?
The state in this case can only ever be BPF_ALU_NON_POINTER, any other
setting from sanitize_val_alu() would be a violation.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf] bpf: fix sanitation rewrite in case of non-pointers
2019-03-01 23:22 ` Daniel Borkmann
@ 2019-03-01 23:42 ` Song Liu
0 siblings, 0 replies; 9+ messages in thread
From: Song Liu @ 2019-03-01 23:42 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Alexei Starovoitov, afabre, Marek Majkowski, bpf, Networking
On Fri, Mar 1, 2019 at 3:23 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 03/02/2019 12:18 AM, Song Liu wrote:
> > On Fri, Mar 1, 2019 at 1:06 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >>
> >> Marek reported that he saw an issue with the below snippet in that
> >> timing measurements where off when loaded as unpriv while results
> >> were reasonable when loaded as privileged:
> >>
> >> [...]
> >> uint64_t a = bpf_ktime_get_ns();
> >> uint64_t b = bpf_ktime_get_ns();
> >> uint64_t delta = b - a;
> >> if ((int64_t)delta > 0) {
> >> [...]
> >>
> >> Turns out there is a bug where a corner case is missing in the fix
> >> d3bd7413e0ca ("bpf: fix sanitation of alu op with pointer / scalar
> >> type from different paths"), namely fixup_bpf_calls() only checks
> >> whether aux has a non-zero alu_state, but it also needs to test for
> >> the case of BPF_ALU_NON_POINTER since in both occasions we need to
> >> skip the masking rewrite (as there is nothing to mask).
> >>
> >> Fixes: d3bd7413e0ca ("bpf: fix sanitation of alu op with pointer / scalar type from different paths")
> >> Reported-by: Marek Majkowski <marek@cloudflare.com>
> >> Reported-by: Arthur Fabre <afabre@cloudflare.com>
> >> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> >> Link: https://lore.kernel.org/netdev/CAJPywTJqP34cK20iLM5YmUMz9KXQOdu1-+BZrGMAGgLuBWz7fg@mail.gmail.com/T/
> >> ---
> >> [ Test case will be routed via bpf-next to avoid useless merge churn
> >> due to test_verifier rework in bpf-next. ]
> >>
> >> kernel/bpf/verifier.c | 3 ++-
> >> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> >> index 8f295b790297..5fcce2f4209d 100644
> >> --- a/kernel/bpf/verifier.c
> >> +++ b/kernel/bpf/verifier.c
> >> @@ -6920,7 +6920,8 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
> >> u32 off_reg;
> >>
> >> aux = &env->insn_aux_data[i + delta];
> >> - if (!aux->alu_state)
> >> + if (!aux->alu_state ||
> >> + aux->alu_state == BPF_ALU_NON_POINTER)
> >
> > alu_state is a bitmap. Shall we check "aux->alu_state &
> > BPF_ALU_NON_POINTER" here?
>
> The state in this case can only ever be BPF_ALU_NON_POINTER, any other
> setting from sanitize_val_alu() would be a violation.
I see. Thanks for the explanation.
Acked-by: Song Liu <songliubraving@fb.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf] bpf: fix sanitation rewrite in case of non-pointers
2019-03-01 21:05 [PATCH bpf] bpf: fix sanitation rewrite in case of non-pointers Daniel Borkmann
2019-03-01 23:18 ` Song Liu
@ 2019-03-02 5:30 ` Alexei Starovoitov
2019-03-02 10:11 ` Sergei Shtylyov
2019-03-05 14:12 ` Jakub Sitnicki
3 siblings, 0 replies; 9+ messages in thread
From: Alexei Starovoitov @ 2019-03-02 5:30 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: ast, afabre, marek, bpf, netdev
On Fri, Mar 01, 2019 at 10:05:29PM +0100, Daniel Borkmann wrote:
> Marek reported that he saw an issue with the below snippet in that
> timing measurements where off when loaded as unpriv while results
> were reasonable when loaded as privileged:
>
> [...]
> uint64_t a = bpf_ktime_get_ns();
> uint64_t b = bpf_ktime_get_ns();
> uint64_t delta = b - a;
> if ((int64_t)delta > 0) {
> [...]
>
> Turns out there is a bug where a corner case is missing in the fix
> d3bd7413e0ca ("bpf: fix sanitation of alu op with pointer / scalar
> type from different paths"), namely fixup_bpf_calls() only checks
> whether aux has a non-zero alu_state, but it also needs to test for
> the case of BPF_ALU_NON_POINTER since in both occasions we need to
> skip the masking rewrite (as there is nothing to mask).
>
> Fixes: d3bd7413e0ca ("bpf: fix sanitation of alu op with pointer / scalar type from different paths")
> Reported-by: Marek Majkowski <marek@cloudflare.com>
> Reported-by: Arthur Fabre <afabre@cloudflare.com>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Link: https://lore.kernel.org/netdev/CAJPywTJqP34cK20iLM5YmUMz9KXQOdu1-+BZrGMAGgLuBWz7fg@mail.gmail.com/T/
Applied, Thanks
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf] bpf: fix sanitation rewrite in case of non-pointers
2019-03-01 21:05 [PATCH bpf] bpf: fix sanitation rewrite in case of non-pointers Daniel Borkmann
2019-03-01 23:18 ` Song Liu
2019-03-02 5:30 ` Alexei Starovoitov
@ 2019-03-02 10:11 ` Sergei Shtylyov
2019-03-05 14:12 ` Jakub Sitnicki
3 siblings, 0 replies; 9+ messages in thread
From: Sergei Shtylyov @ 2019-03-02 10:11 UTC (permalink / raw)
To: Daniel Borkmann, ast; +Cc: afabre, marek, bpf, netdev
Hello!
On 02.03.2019 0:05, Daniel Borkmann wrote:
> Marek reported that he saw an issue with the below snippet in that
> timing measurements where off when loaded as unpriv while results
Were?
> were reasonable when loaded as privileged:
>
> [...]
> uint64_t a = bpf_ktime_get_ns();
> uint64_t b = bpf_ktime_get_ns();
> uint64_t delta = b - a;
> if ((int64_t)delta > 0) {
> [...]
>
> Turns out there is a bug where a corner case is missing in the fix
> d3bd7413e0ca ("bpf: fix sanitation of alu op with pointer / scalar
> type from different paths"), namely fixup_bpf_calls() only checks
> whether aux has a non-zero alu_state, but it also needs to test for
> the case of BPF_ALU_NON_POINTER since in both occasions we need to
> skip the masking rewrite (as there is nothing to mask).
>
> Fixes: d3bd7413e0ca ("bpf: fix sanitation of alu op with pointer / scalar type from different paths")
> Reported-by: Marek Majkowski <marek@cloudflare.com>
> Reported-by: Arthur Fabre <afabre@cloudflare.com>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Link: https://lore.kernel.org/netdev/CAJPywTJqP34cK20iLM5YmUMz9KXQOdu1-+BZrGMAGgLuBWz7fg@mail.gmail.com/T/
[...]
MBR, Sergei
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf] bpf: fix sanitation rewrite in case of non-pointers
2019-03-01 21:05 [PATCH bpf] bpf: fix sanitation rewrite in case of non-pointers Daniel Borkmann
` (2 preceding siblings ...)
2019-03-02 10:11 ` Sergei Shtylyov
@ 2019-03-05 14:12 ` Jakub Sitnicki
2019-03-05 14:30 ` Daniel Borkmann
3 siblings, 1 reply; 9+ messages in thread
From: Jakub Sitnicki @ 2019-03-05 14:12 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: ast, afabre, marek, bpf, netdev
On Fri, Mar 01, 2019 at 10:05 PM CET, Daniel Borkmann wrote:
> Marek reported that he saw an issue with the below snippet in that
> timing measurements where off when loaded as unpriv while results
> were reasonable when loaded as privileged:
>
> [...]
> uint64_t a = bpf_ktime_get_ns();
> uint64_t b = bpf_ktime_get_ns();
> uint64_t delta = b - a;
> if ((int64_t)delta > 0) {
> [...]
>
> Turns out there is a bug where a corner case is missing in the fix
> d3bd7413e0ca ("bpf: fix sanitation of alu op with pointer / scalar
> type from different paths"), namely fixup_bpf_calls() only checks
> whether aux has a non-zero alu_state, but it also needs to test for
> the case of BPF_ALU_NON_POINTER since in both occasions we need to
> skip the masking rewrite (as there is nothing to mask).
>
> Fixes: d3bd7413e0ca ("bpf: fix sanitation of alu op with pointer / scalar type from different paths")
> Reported-by: Marek Majkowski <marek@cloudflare.com>
> Reported-by: Arthur Fabre <afabre@cloudflare.com>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Link: https://lore.kernel.org/netdev/CAJPywTJqP34cK20iLM5YmUMz9KXQOdu1-+BZrGMAGgLuBWz7fg@mail.gmail.com/T/
> ---
Could you please queue it for -stable which has d3bd7413e0ca ("bpf: fix
sanitation of alu op with pointer / scalar type from different paths")?
Thanks,
Jakub
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf] bpf: fix sanitation rewrite in case of non-pointers
2019-03-05 14:12 ` Jakub Sitnicki
@ 2019-03-05 14:30 ` Daniel Borkmann
2019-03-06 8:59 ` Jakub Sitnicki
0 siblings, 1 reply; 9+ messages in thread
From: Daniel Borkmann @ 2019-03-05 14:30 UTC (permalink / raw)
To: Jakub Sitnicki; +Cc: ast, afabre, marek, bpf, netdev
On 03/05/2019 03:12 PM, Jakub Sitnicki wrote:
[...]
> Could you please queue it for -stable which has d3bd7413e0ca ("bpf: fix
> sanitation of alu op with pointer / scalar type from different paths")?
Already done here yesterday morning:
https://lore.kernel.org/stable/40b25ec1c31e234cf7eee75d62083a9a4bcbdbfe.1551702973.git.daniel@iogearbox.net/T/#u
> Thanks,
> Jakub
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf] bpf: fix sanitation rewrite in case of non-pointers
2019-03-05 14:30 ` Daniel Borkmann
@ 2019-03-06 8:59 ` Jakub Sitnicki
0 siblings, 0 replies; 9+ messages in thread
From: Jakub Sitnicki @ 2019-03-06 8:59 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: ast, afabre, marek, bpf, netdev
On Tue, Mar 05, 2019 at 03:30 PM CET, Daniel Borkmann wrote:
> On 03/05/2019 03:12 PM, Jakub Sitnicki wrote:
> [...]
>> Could you please queue it for -stable which has d3bd7413e0ca ("bpf: fix
>> sanitation of alu op with pointer / scalar type from different paths")?
>
> Already done here yesterday morning:
>
> https://lore.kernel.org/stable/40b25ec1c31e234cf7eee75d62083a9a4bcbdbfe.1551702973.git.daniel@iogearbox.net/T/#u
Ah, you're one step ahead of me. Thank you.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-03-06 8:59 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-01 21:05 [PATCH bpf] bpf: fix sanitation rewrite in case of non-pointers Daniel Borkmann
2019-03-01 23:18 ` Song Liu
2019-03-01 23:22 ` Daniel Borkmann
2019-03-01 23:42 ` Song Liu
2019-03-02 5:30 ` Alexei Starovoitov
2019-03-02 10:11 ` Sergei Shtylyov
2019-03-05 14:12 ` Jakub Sitnicki
2019-03-05 14:30 ` Daniel Borkmann
2019-03-06 8:59 ` Jakub Sitnicki
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.