* [PATCH v2 net-next] bpf: enable verifier to add 0 to packet ptr
@ 2017-02-03 17:22 William Tu
2017-02-03 20:39 ` Daniel Borkmann
2017-02-03 20:55 ` Alexei Starovoitov
0 siblings, 2 replies; 7+ messages in thread
From: William Tu @ 2017-02-03 17:22 UTC (permalink / raw)
To: netdev; +Cc: Mihai Budiu
The patch fixes the case when adding a zero value to the packet
pointer. The verifer reports the following error:
[...]
R0=imm0,min_value=0,max_value=0
R1=pkt(id=0,off=0,r=4)
R2=pkt_end R3=fp-12
R4=imm4,min_value=4,max_value=4
R5=pkt(id=0,off=4,r=4)
269: (bf) r2 = r0 // r2 becomes imm0
270: (77) r2 >>= 3
271: (bf) r4 = r1 // r4 becomes pkt ptr
272: (0f) r4 += r2 // r4 += 0
addition of negative constant to packet pointer is not allowed
Signed-off-by: William Tu <u9012063@gmail.com>
Signed-off-by: Mihai Budiu <mbudiu@vmware.com>
---
kernel/bpf/verifier.c | 2 +-
tools/testing/selftests/bpf/test_verifier.c | 15 +++++++++++++++
2 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index fb3513b..1a754e5 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -1397,7 +1397,7 @@ static int check_packet_ptr_add(struct bpf_verifier_env *env,
imm = insn->imm;
add_imm:
- if (imm <= 0) {
+ if (imm < 0) {
verbose("addition of negative constant to packet pointer is not allowed\n");
return -EACCES;
}
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 0d0912c..a2b5c7e 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -2404,6 +2404,21 @@ static struct bpf_test tests[] = {
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
},
{
+ "direct packet access: test14 (pkt_ptr += 0, good access)",
+ .insns = {
+ BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1,
+ offsetof(struct __sk_buff, data)),
+ BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_1,
+ offsetof(struct __sk_buff, data_end)),
+ BPF_MOV64_REG(BPF_REG_0, BPF_REG_2),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 0),
+ BPF_MOV64_IMM(BPF_REG_0, 1),
+ BPF_EXIT_INSN(),
+ },
+ .result = ACCEPT,
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ },
+ {
"helper access to packet: test1, valid packet_ptr range",
.insns = {
BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1,
--
2.7.4
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2 net-next] bpf: enable verifier to add 0 to packet ptr
2017-02-03 17:22 [PATCH v2 net-next] bpf: enable verifier to add 0 to packet ptr William Tu
@ 2017-02-03 20:39 ` Daniel Borkmann
2017-02-03 20:55 ` Alexei Starovoitov
1 sibling, 0 replies; 7+ messages in thread
From: Daniel Borkmann @ 2017-02-03 20:39 UTC (permalink / raw)
To: William Tu; +Cc: netdev, Mihai Budiu, ast
[ please keep us Cc'ed ;) ]
On 02/03/2017 06:22 PM, William Tu wrote:
> The patch fixes the case when adding a zero value to the packet
> pointer. The verifer reports the following error:
> [...]
> R0=imm0,min_value=0,max_value=0
> R1=pkt(id=0,off=0,r=4)
> R2=pkt_end R3=fp-12
> R4=imm4,min_value=4,max_value=4
> R5=pkt(id=0,off=4,r=4)
> 269: (bf) r2 = r0 // r2 becomes imm0
> 270: (77) r2 >>= 3
> 271: (bf) r4 = r1 // r4 becomes pkt ptr
> 272: (0f) r4 += r2 // r4 += 0
> addition of negative constant to packet pointer is not allowed
>
> Signed-off-by: William Tu <u9012063@gmail.com>
> Signed-off-by: Mihai Budiu <mbudiu@vmware.com>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
Thanks!
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 net-next] bpf: enable verifier to add 0 to packet ptr
2017-02-03 17:22 [PATCH v2 net-next] bpf: enable verifier to add 0 to packet ptr William Tu
2017-02-03 20:39 ` Daniel Borkmann
@ 2017-02-03 20:55 ` Alexei Starovoitov
2017-02-03 21:10 ` William Tu
1 sibling, 1 reply; 7+ messages in thread
From: Alexei Starovoitov @ 2017-02-03 20:55 UTC (permalink / raw)
To: William Tu; +Cc: netdev, Mihai Budiu
On Fri, Feb 03, 2017 at 09:22:45AM -0800, William Tu wrote:
> The patch fixes the case when adding a zero value to the packet
> pointer. The verifer reports the following error:
> [...]
> R0=imm0,min_value=0,max_value=0
> R1=pkt(id=0,off=0,r=4)
> R2=pkt_end R3=fp-12
> R4=imm4,min_value=4,max_value=4
> R5=pkt(id=0,off=4,r=4)
> 269: (bf) r2 = r0 // r2 becomes imm0
> 270: (77) r2 >>= 3
> 271: (bf) r4 = r1 // r4 becomes pkt ptr
> 272: (0f) r4 += r2 // r4 += 0
> addition of negative constant to packet pointer is not allowed
>
> Signed-off-by: William Tu <u9012063@gmail.com>
> Signed-off-by: Mihai Budiu <mbudiu@vmware.com>
> ---
> kernel/bpf/verifier.c | 2 +-
> tools/testing/selftests/bpf/test_verifier.c | 15 +++++++++++++++
> 2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index fb3513b..1a754e5 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -1397,7 +1397,7 @@ static int check_packet_ptr_add(struct bpf_verifier_env *env,
> imm = insn->imm;
>
> add_imm:
> - if (imm <= 0) {
> + if (imm < 0) {
> verbose("addition of negative constant to packet pointer is not allowed\n");
> return -EACCES;
> }
> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> index 0d0912c..a2b5c7e 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -2404,6 +2404,21 @@ static struct bpf_test tests[] = {
> .prog_type = BPF_PROG_TYPE_SCHED_CLS,
> },
> {
> + "direct packet access: test14 (pkt_ptr += 0, good access)",
> + .insns = {
> + BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1,
> + offsetof(struct __sk_buff, data)),
> + BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_1,
> + offsetof(struct __sk_buff, data_end)),
> + BPF_MOV64_REG(BPF_REG_0, BPF_REG_2),
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 0),
wait. the test is bogus.
please write the proper test for the feature
and check that it fails before the patch and passes afterwards.
> + BPF_MOV64_IMM(BPF_REG_0, 1),
> + BPF_EXIT_INSN(),
> + },
> + .result = ACCEPT,
> + .prog_type = BPF_PROG_TYPE_SCHED_CLS,
> + },
> + {
> "helper access to packet: test1, valid packet_ptr range",
> .insns = {
> BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1,
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2 net-next] bpf: enable verifier to add 0 to packet ptr
2017-02-03 20:55 ` Alexei Starovoitov
@ 2017-02-03 21:10 ` William Tu
2017-02-03 22:29 ` Daniel Borkmann
0 siblings, 1 reply; 7+ messages in thread
From: William Tu @ 2017-02-03 21:10 UTC (permalink / raw)
To: Alexei Starovoitov; +Cc: Linux Kernel Network Developers, Mihai Budiu
Hi Alexei,
why it is bogus? on my system, it fails without the patch applied.
--William
On Fri, Feb 3, 2017 at 12:55 PM, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
> On Fri, Feb 03, 2017 at 09:22:45AM -0800, William Tu wrote:
>> The patch fixes the case when adding a zero value to the packet
>> pointer. The verifer reports the following error:
>> [...]
>> R0=imm0,min_value=0,max_value=0
>> R1=pkt(id=0,off=0,r=4)
>> R2=pkt_end R3=fp-12
>> R4=imm4,min_value=4,max_value=4
>> R5=pkt(id=0,off=4,r=4)
>> 269: (bf) r2 = r0 // r2 becomes imm0
>> 270: (77) r2 >>= 3
>> 271: (bf) r4 = r1 // r4 becomes pkt ptr
>> 272: (0f) r4 += r2 // r4 += 0
>> addition of negative constant to packet pointer is not allowed
>>
>> Signed-off-by: William Tu <u9012063@gmail.com>
>> Signed-off-by: Mihai Budiu <mbudiu@vmware.com>
>> ---
>> kernel/bpf/verifier.c | 2 +-
>> tools/testing/selftests/bpf/test_verifier.c | 15 +++++++++++++++
>> 2 files changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index fb3513b..1a754e5 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -1397,7 +1397,7 @@ static int check_packet_ptr_add(struct bpf_verifier_env *env,
>> imm = insn->imm;
>>
>> add_imm:
>> - if (imm <= 0) {
>> + if (imm < 0) {
>> verbose("addition of negative constant to packet pointer is not allowed\n");
>> return -EACCES;
>> }
>> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
>> index 0d0912c..a2b5c7e 100644
>> --- a/tools/testing/selftests/bpf/test_verifier.c
>> +++ b/tools/testing/selftests/bpf/test_verifier.c
>> @@ -2404,6 +2404,21 @@ static struct bpf_test tests[] = {
>> .prog_type = BPF_PROG_TYPE_SCHED_CLS,
>> },
>> {
>> + "direct packet access: test14 (pkt_ptr += 0, good access)",
>> + .insns = {
>> + BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1,
>> + offsetof(struct __sk_buff, data)),
>> + BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_1,
>> + offsetof(struct __sk_buff, data_end)),
>> + BPF_MOV64_REG(BPF_REG_0, BPF_REG_2),
>> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 0),
>
> wait. the test is bogus.
> please write the proper test for the feature
> and check that it fails before the patch and passes afterwards.
>
>> + BPF_MOV64_IMM(BPF_REG_0, 1),
>> + BPF_EXIT_INSN(),
>> + },
>> + .result = ACCEPT,
>> + .prog_type = BPF_PROG_TYPE_SCHED_CLS,
>> + },
>> + {
>> "helper access to packet: test1, valid packet_ptr range",
>> .insns = {
>> BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1,
>> --
>> 2.7.4
>>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2 net-next] bpf: enable verifier to add 0 to packet ptr
2017-02-03 21:10 ` William Tu
@ 2017-02-03 22:29 ` Daniel Borkmann
2017-02-03 22:53 ` Alexei Starovoitov
0 siblings, 1 reply; 7+ messages in thread
From: Daniel Borkmann @ 2017-02-03 22:29 UTC (permalink / raw)
To: William Tu, Alexei Starovoitov
Cc: Linux Kernel Network Developers, Mihai Budiu
On 02/03/2017 10:10 PM, William Tu wrote:
> Hi Alexei,
>
> why it is bogus? on my system, it fails without the patch applied.
>
> --William
>
> On Fri, Feb 3, 2017 at 12:55 PM, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>> On Fri, Feb 03, 2017 at 09:22:45AM -0800, William Tu wrote:
>>> The patch fixes the case when adding a zero value to the packet
>>> pointer. The verifer reports the following error:
>>> [...]
>>> R0=imm0,min_value=0,max_value=0
>>> R1=pkt(id=0,off=0,r=4)
>>> R2=pkt_end R3=fp-12
>>> R4=imm4,min_value=4,max_value=4
>>> R5=pkt(id=0,off=4,r=4)
>>> 269: (bf) r2 = r0 // r2 becomes imm0
>>> 270: (77) r2 >>= 3
>>> 271: (bf) r4 = r1 // r4 becomes pkt ptr
>>> 272: (0f) r4 += r2 // r4 += 0
>>> addition of negative constant to packet pointer is not allowed
>>>
>>> Signed-off-by: William Tu <u9012063@gmail.com>
>>> Signed-off-by: Mihai Budiu <mbudiu@vmware.com>
[...]
>>> {
>>> + "direct packet access: test14 (pkt_ptr += 0, good access)",
>>> + .insns = {
>>> + BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1,
>>> + offsetof(struct __sk_buff, data)),
>>> + BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_1,
>>> + offsetof(struct __sk_buff, data_end)),
>>> + BPF_MOV64_REG(BPF_REG_0, BPF_REG_2),
>>> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 0),
>>
>> wait. the test is bogus.
>> please write the proper test for the feature
>> and check that it fails before the patch and passes afterwards.
But still same code path that is executed in verifier as BPF_K and
CONST_IMM tracked reg both share the same path under add_imm label
in check_packet_ptr_add(), no? So it becomes r2=pkt(id=0,off=0,r=0);
r0 = r2; r0 += 0 here in this test. Probably okay as well, though
there could be risk that in future both don't share the same path
for some reason. I guess you were referring to either adding tests
for BPF_K /and/ CONST_IMM reg or just the latter, right?
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2 net-next] bpf: enable verifier to add 0 to packet ptr
2017-02-03 22:29 ` Daniel Borkmann
@ 2017-02-03 22:53 ` Alexei Starovoitov
2017-02-03 23:28 ` William Tu
0 siblings, 1 reply; 7+ messages in thread
From: Alexei Starovoitov @ 2017-02-03 22:53 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: William Tu, Linux Kernel Network Developers, Mihai Budiu
On Fri, Feb 03, 2017 at 11:29:19PM +0100, Daniel Borkmann wrote:
> On 02/03/2017 10:10 PM, William Tu wrote:
> >Hi Alexei,
> >
> >why it is bogus? on my system, it fails without the patch applied.
> >
> >--William
> >
> >On Fri, Feb 3, 2017 at 12:55 PM, Alexei Starovoitov
> ><alexei.starovoitov@gmail.com> wrote:
> >>On Fri, Feb 03, 2017 at 09:22:45AM -0800, William Tu wrote:
> >>>The patch fixes the case when adding a zero value to the packet
> >>>pointer. The verifer reports the following error:
> >>> [...]
> >>> R0=imm0,min_value=0,max_value=0
> >>> R1=pkt(id=0,off=0,r=4)
> >>> R2=pkt_end R3=fp-12
> >>> R4=imm4,min_value=4,max_value=4
> >>> R5=pkt(id=0,off=4,r=4)
> >>> 269: (bf) r2 = r0 // r2 becomes imm0
> >>> 270: (77) r2 >>= 3
> >>> 271: (bf) r4 = r1 // r4 becomes pkt ptr
> >>> 272: (0f) r4 += r2 // r4 += 0
> >>> addition of negative constant to packet pointer is not allowed
> >>>
> >>>Signed-off-by: William Tu <u9012063@gmail.com>
> >>>Signed-off-by: Mihai Budiu <mbudiu@vmware.com>
> [...]
> >>> {
> >>>+ "direct packet access: test14 (pkt_ptr += 0, good access)",
> >>>+ .insns = {
> >>>+ BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1,
> >>>+ offsetof(struct __sk_buff, data)),
> >>>+ BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_1,
> >>>+ offsetof(struct __sk_buff, data_end)),
> >>>+ BPF_MOV64_REG(BPF_REG_0, BPF_REG_2),
> >>>+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 0),
> >>
> >>wait. the test is bogus.
> >>please write the proper test for the feature
> >>and check that it fails before the patch and passes afterwards.
>
> But still same code path that is executed in verifier as BPF_K and
> CONST_IMM tracked reg both share the same path under add_imm label
> in check_packet_ptr_add(), no? So it becomes r2=pkt(id=0,off=0,r=0);
> r0 = r2; r0 += 0 here in this test. Probably okay as well, though
> there could be risk that in future both don't share the same path
> for some reason. I guess you were referring to either adding tests
> for BPF_K /and/ CONST_IMM reg or just the latter, right?
yes. Sorry I wasn't clear.
imo the 'r0 += 0' is not something that verifier should recognize,
since such nop insns shouldn't be generated by the compiler.
It happened that the code path in verifier covers that case
as well, but I think we really need to test 'rX += rY' case
where rY is recognized as imm0, since that what the original
use case was about.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2 net-next] bpf: enable verifier to add 0 to packet ptr
2017-02-03 22:53 ` Alexei Starovoitov
@ 2017-02-03 23:28 ` William Tu
0 siblings, 0 replies; 7+ messages in thread
From: William Tu @ 2017-02-03 23:28 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Daniel Borkmann, Linux Kernel Network Developers, Mihai Budiu
Thanks. I got it. I will resubmit v3 patch!
On Fri, Feb 3, 2017 at 2:53 PM, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
> On Fri, Feb 03, 2017 at 11:29:19PM +0100, Daniel Borkmann wrote:
>> On 02/03/2017 10:10 PM, William Tu wrote:
>> >Hi Alexei,
>> >
>> >why it is bogus? on my system, it fails without the patch applied.
>> >
>> >--William
>> >
>> >On Fri, Feb 3, 2017 at 12:55 PM, Alexei Starovoitov
>> ><alexei.starovoitov@gmail.com> wrote:
>> >>On Fri, Feb 03, 2017 at 09:22:45AM -0800, William Tu wrote:
>> >>>The patch fixes the case when adding a zero value to the packet
>> >>>pointer. The verifer reports the following error:
>> >>> [...]
>> >>> R0=imm0,min_value=0,max_value=0
>> >>> R1=pkt(id=0,off=0,r=4)
>> >>> R2=pkt_end R3=fp-12
>> >>> R4=imm4,min_value=4,max_value=4
>> >>> R5=pkt(id=0,off=4,r=4)
>> >>> 269: (bf) r2 = r0 // r2 becomes imm0
>> >>> 270: (77) r2 >>= 3
>> >>> 271: (bf) r4 = r1 // r4 becomes pkt ptr
>> >>> 272: (0f) r4 += r2 // r4 += 0
>> >>> addition of negative constant to packet pointer is not allowed
>> >>>
>> >>>Signed-off-by: William Tu <u9012063@gmail.com>
>> >>>Signed-off-by: Mihai Budiu <mbudiu@vmware.com>
>> [...]
>> >>> {
>> >>>+ "direct packet access: test14 (pkt_ptr += 0, good access)",
>> >>>+ .insns = {
>> >>>+ BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1,
>> >>>+ offsetof(struct __sk_buff, data)),
>> >>>+ BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_1,
>> >>>+ offsetof(struct __sk_buff, data_end)),
>> >>>+ BPF_MOV64_REG(BPF_REG_0, BPF_REG_2),
>> >>>+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 0),
>> >>
>> >>wait. the test is bogus.
>> >>please write the proper test for the feature
>> >>and check that it fails before the patch and passes afterwards.
>>
>> But still same code path that is executed in verifier as BPF_K and
>> CONST_IMM tracked reg both share the same path under add_imm label
>> in check_packet_ptr_add(), no? So it becomes r2=pkt(id=0,off=0,r=0);
>> r0 = r2; r0 += 0 here in this test. Probably okay as well, though
>> there could be risk that in future both don't share the same path
>> for some reason. I guess you were referring to either adding tests
>> for BPF_K /and/ CONST_IMM reg or just the latter, right?
>
> yes. Sorry I wasn't clear.
> imo the 'r0 += 0' is not something that verifier should recognize,
> since such nop insns shouldn't be generated by the compiler.
> It happened that the code path in verifier covers that case
> as well, but I think we really need to test 'rX += rY' case
> where rY is recognized as imm0, since that what the original
> use case was about.
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-02-03 23:29 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-02-03 17:22 [PATCH v2 net-next] bpf: enable verifier to add 0 to packet ptr William Tu
2017-02-03 20:39 ` Daniel Borkmann
2017-02-03 20:55 ` Alexei Starovoitov
2017-02-03 21:10 ` William Tu
2017-02-03 22:29 ` Daniel Borkmann
2017-02-03 22:53 ` Alexei Starovoitov
2017-02-03 23:28 ` William Tu
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).