* 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