* [PATCH v2 bpf 1/2] bpf: verifier: check for packet data access based on target prog
@ 2020-08-21 0:28 Udip Pant
2020-08-21 0:28 ` [PATCH v2 bpf 2/2] selftests/bpf: add test for freplace program with write access Udip Pant
2020-08-21 6:13 ` [PATCH v2 bpf 1/2] bpf: verifier: check for packet data access based on target prog Yonghong Song
0 siblings, 2 replies; 8+ messages in thread
From: Udip Pant @ 2020-08-21 0:28 UTC (permalink / raw)
To: Udip Pant, Alexei Starovoitov, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, David S . Miller
Cc: netdev, bpf, linux-kernel
While using dynamic program extension (of type BPF_PROG_TYPE_EXT), we
need to check the program type of the target program to grant the read /
write access to the packet data.
The BPF_PROG_TYPE_EXT type can be used to extend types such as XDP, SKB
and others. Since the BPF_PROG_TYPE_EXT program type on itself is just a
placeholder for those, we need this extended check for those target
programs to actually work while using this option.
Tested this with a freplace xdp program. Without this patch, the
verifier fails with error 'cannot write into packet'.
Signed-off-by: Udip Pant <udippant@fb.com>
---
kernel/bpf/verifier.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index ef938f17b944..4d7604430994 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -2629,7 +2629,11 @@ static bool may_access_direct_pkt_data(struct bpf_verifier_env *env,
const struct bpf_call_arg_meta *meta,
enum bpf_access_type t)
{
- switch (env->prog->type) {
+ struct bpf_prog *prog = env->prog;
+ enum bpf_prog_type prog_type = prog->aux->linked_prog ?
+ prog->aux->linked_prog->type : prog->type;
+
+ switch (prog_type) {
/* Program types only with direct read access go here! */
case BPF_PROG_TYPE_LWT_IN:
case BPF_PROG_TYPE_LWT_OUT:
--
2.24.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 bpf 2/2] selftests/bpf: add test for freplace program with write access
2020-08-21 0:28 [PATCH v2 bpf 1/2] bpf: verifier: check for packet data access based on target prog Udip Pant
@ 2020-08-21 0:28 ` Udip Pant
2020-08-21 6:19 ` Yonghong Song
2020-08-21 6:13 ` [PATCH v2 bpf 1/2] bpf: verifier: check for packet data access based on target prog Yonghong Song
1 sibling, 1 reply; 8+ messages in thread
From: Udip Pant @ 2020-08-21 0:28 UTC (permalink / raw)
To: Udip Pant, Alexei Starovoitov, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, David S . Miller
Cc: netdev, bpf, linux-kernel
This adds a selftest that tests the behavior when a freplace target program
attempts to make a write access on a packet. The expectation is that the read or write
access is granted based on the program type of the linked program and
not itself (which is of type, for e.g., BPF_PROG_TYPE_EXT).
This test fails without the associated patch on the verifier.
Signed-off-by: Udip Pant <udippant@fb.com>
---
.../selftests/bpf/prog_tests/fexit_bpf2bpf.c | 1 +
.../selftests/bpf/progs/fexit_bpf2bpf.c | 27 +++++++++++++++++++
.../selftests/bpf/progs/test_pkt_access.c | 20 ++++++++++++++
3 files changed, 48 insertions(+)
diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
index 197d0d217b56..7c7168963d52 100644
--- a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
+++ b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
@@ -123,6 +123,7 @@ static void test_func_replace(void)
"freplace/get_skb_len",
"freplace/get_skb_ifindex",
"freplace/get_constant",
+ "freplace/test_pkt_write_access_subprog",
};
test_fexit_bpf2bpf_common("./fexit_bpf2bpf.o",
"./test_pkt_access.o",
diff --git a/tools/testing/selftests/bpf/progs/fexit_bpf2bpf.c b/tools/testing/selftests/bpf/progs/fexit_bpf2bpf.c
index 98e1efe14549..973e8e1a6667 100644
--- a/tools/testing/selftests/bpf/progs/fexit_bpf2bpf.c
+++ b/tools/testing/selftests/bpf/progs/fexit_bpf2bpf.c
@@ -2,7 +2,9 @@
/* Copyright (c) 2019 Facebook */
#include <linux/stddef.h>
#include <linux/ipv6.h>
+#include <linux/if_ether.h>
#include <linux/bpf.h>
+#include <linux/tcp.h>
#include <bpf/bpf_helpers.h>
#include <bpf/bpf_endian.h>
#include <bpf/bpf_tracing.h>
@@ -151,4 +153,29 @@ int new_get_constant(long val)
test_get_constant = 1;
return test_get_constant; /* original get_constant() returns val - 122 */
}
+
+__u64 test_pkt_write_access_subprog = 0;
+SEC("freplace/test_pkt_write_access_subprog")
+int new_test_pkt_write_access_subprog(struct __sk_buff *skb, __u32 off)
+{
+
+ void *data = (void *)(long)skb->data;
+ void *data_end = (void *)(long)skb->data_end;
+ struct tcphdr *tcp;
+
+ if (off > sizeof(struct ethhdr) + sizeof(struct ipv6hdr))
+ return -1;
+
+ tcp = data + off;
+ if (tcp + 1 > data_end)
+ return -1;
+
+ /* make modifications to the packet data */
+ tcp->check++;
+ tcp->syn = 0;
+
+ test_pkt_write_access_subprog = 1;
+ return 0;
+}
+
char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/test_pkt_access.c b/tools/testing/selftests/bpf/progs/test_pkt_access.c
index e72eba4a93d2..c36a4731c8e0 100644
--- a/tools/testing/selftests/bpf/progs/test_pkt_access.c
+++ b/tools/testing/selftests/bpf/progs/test_pkt_access.c
@@ -79,6 +79,24 @@ int get_skb_ifindex(int val, struct __sk_buff *skb, int var)
return skb->ifindex * val * var;
}
+__attribute__ ((noinline))
+int test_pkt_write_access_subprog(struct __sk_buff *skb, __u32 off)
+{
+ void *data = (void *)(long)skb->data;
+ void *data_end = (void *)(long)skb->data_end;
+ struct tcphdr *tcp = NULL;
+
+ if (off > sizeof(struct ethhdr) + sizeof(struct ipv6hdr))
+ return -1;
+
+ tcp = data + off;
+ if (tcp + 1 > data_end)
+ return -1;
+ /* make modification to the packet data */
+ tcp->check = tcp->check + 1;
+ return 0;
+}
+
SEC("classifier/test_pkt_access")
int test_pkt_access(struct __sk_buff *skb)
{
@@ -117,6 +135,8 @@ int test_pkt_access(struct __sk_buff *skb)
if (test_pkt_access_subprog3(3, skb) != skb->len * 3 * skb->ifindex)
return TC_ACT_SHOT;
if (tcp) {
+ if (test_pkt_write_access_subprog(skb, (void *)tcp - data))
+ return TC_ACT_SHOT;
if (((void *)(tcp) + 20) > data_end || proto != 6)
return TC_ACT_SHOT;
barrier(); /* to force ordering of checks */
--
2.24.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 bpf 1/2] bpf: verifier: check for packet data access based on target prog
2020-08-21 0:28 [PATCH v2 bpf 1/2] bpf: verifier: check for packet data access based on target prog Udip Pant
2020-08-21 0:28 ` [PATCH v2 bpf 2/2] selftests/bpf: add test for freplace program with write access Udip Pant
@ 2020-08-21 6:13 ` Yonghong Song
2020-08-21 6:17 ` Yonghong Song
1 sibling, 1 reply; 8+ messages in thread
From: Yonghong Song @ 2020-08-21 6:13 UTC (permalink / raw)
To: Udip Pant, Alexei Starovoitov, Martin KaFai Lau, Song Liu,
Andrii Nakryiko, David S . Miller
Cc: netdev, bpf, linux-kernel
On 8/20/20 5:28 PM, Udip Pant wrote:
> While using dynamic program extension (of type BPF_PROG_TYPE_EXT), we
> need to check the program type of the target program to grant the read /
> write access to the packet data.
>
> The BPF_PROG_TYPE_EXT type can be used to extend types such as XDP, SKB
> and others. Since the BPF_PROG_TYPE_EXT program type on itself is just a
> placeholder for those, we need this extended check for those target
> programs to actually work while using this option.
>
> Tested this with a freplace xdp program. Without this patch, the
> verifier fails with error 'cannot write into packet'.
>
> Signed-off-by: Udip Pant <udippant@fb.com>
> ---
> kernel/bpf/verifier.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index ef938f17b944..4d7604430994 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -2629,7 +2629,11 @@ static bool may_access_direct_pkt_data(struct bpf_verifier_env *env,
> const struct bpf_call_arg_meta *meta,
> enum bpf_access_type t)
> {
> - switch (env->prog->type) {
> + struct bpf_prog *prog = env->prog;
> + enum bpf_prog_type prog_type = prog->aux->linked_prog ?
> + prog->aux->linked_prog->type : prog->type;
I checked the verifier code. There are several places where
prog->type is checked and EXT program type will behave differently
from the linked program.
Maybe abstract the the above logic to one static function like
static enum bpf_prog_type resolved_prog_type(struct bpf_prog *prog)
{
return prog->aux->linked_prog ? prog->aux->linked_prog->type
: prog->type;
}
This function can then be used in different places to give the resolved
prog type.
Besides here checking pkt access permission,
another possible places to consider is return value
in function check_return_code(). Currently,
for EXT program, the result value can be anything. This may need to
be enforced. Could you take a look? It could be others as well.
You can take a look at verifier.c by searching "prog->type".
> +
> + switch (prog_type) {
> /* Program types only with direct read access go here! */
> case BPF_PROG_TYPE_LWT_IN:
> case BPF_PROG_TYPE_LWT_OUT:
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 bpf 1/2] bpf: verifier: check for packet data access based on target prog
2020-08-21 6:13 ` [PATCH v2 bpf 1/2] bpf: verifier: check for packet data access based on target prog Yonghong Song
@ 2020-08-21 6:17 ` Yonghong Song
2020-08-21 19:07 ` Udip Pant
0 siblings, 1 reply; 8+ messages in thread
From: Yonghong Song @ 2020-08-21 6:17 UTC (permalink / raw)
To: Udip Pant, Alexei Starovoitov, Martin KaFai Lau, Song Liu,
Andrii Nakryiko, David S . Miller
Cc: netdev, bpf, linux-kernel
On 8/20/20 11:13 PM, Yonghong Song wrote:
>
>
> On 8/20/20 5:28 PM, Udip Pant wrote:
>> While using dynamic program extension (of type BPF_PROG_TYPE_EXT), we
>> need to check the program type of the target program to grant the read /
>> write access to the packet data.
>>
>> The BPF_PROG_TYPE_EXT type can be used to extend types such as XDP, SKB
>> and others. Since the BPF_PROG_TYPE_EXT program type on itself is just a
>> placeholder for those, we need this extended check for those target
>> programs to actually work while using this option.
>>
>> Tested this with a freplace xdp program. Without this patch, the
>> verifier fails with error 'cannot write into packet'.
>>
>> Signed-off-by: Udip Pant <udippant@fb.com>
>> ---
>> kernel/bpf/verifier.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index ef938f17b944..4d7604430994 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -2629,7 +2629,11 @@ static bool may_access_direct_pkt_data(struct
>> bpf_verifier_env *env,
>> const struct bpf_call_arg_meta *meta,
>> enum bpf_access_type t)
>> {
>> - switch (env->prog->type) {
>> + struct bpf_prog *prog = env->prog;
>> + enum bpf_prog_type prog_type = prog->aux->linked_prog ?
>> + prog->aux->linked_prog->type : prog->type;
>
> I checked the verifier code. There are several places where
> prog->type is checked and EXT program type will behave differently
> from the linked program.
>
> Maybe abstract the the above logic to one static function like
>
> static enum bpf_prog_type resolved_prog_type(struct bpf_prog *prog)
> {
> return prog->aux->linked_prog ? prog->aux->linked_prog->type
> : prog->type;
> }
>
> This function can then be used in different places to give the resolved
> prog type.
>
> Besides here checking pkt access permission,
> another possible places to consider is return value
> in function check_return_code(). Currently,
> for EXT program, the result value can be anything. This may need to
> be enforced. Could you take a look? It could be others as well.
> You can take a look at verifier.c by searching "prog->type".
Note that if the EXT program tries to replace a global subprogram,
then return value cannot be enforced, just as what Patch #2 example shows.
>
>> +
>> + switch (prog_type) {
>> /* Program types only with direct read access go here! */
>> case BPF_PROG_TYPE_LWT_IN:
>> case BPF_PROG_TYPE_LWT_OUT:
>>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 bpf 2/2] selftests/bpf: add test for freplace program with write access
2020-08-21 0:28 ` [PATCH v2 bpf 2/2] selftests/bpf: add test for freplace program with write access Udip Pant
@ 2020-08-21 6:19 ` Yonghong Song
0 siblings, 0 replies; 8+ messages in thread
From: Yonghong Song @ 2020-08-21 6:19 UTC (permalink / raw)
To: Udip Pant, Alexei Starovoitov, Martin KaFai Lau, Song Liu,
Andrii Nakryiko, David S . Miller
Cc: netdev, bpf, linux-kernel
On 8/20/20 5:28 PM, Udip Pant wrote:
> This adds a selftest that tests the behavior when a freplace target program
> attempts to make a write access on a packet. The expectation is that the read or write
> access is granted based on the program type of the linked program and
> not itself (which is of type, for e.g., BPF_PROG_TYPE_EXT).
>
> This test fails without the associated patch on the verifier.
>
> Signed-off-by: Udip Pant <udippant@fb.com>
> ---
Acked-by: Yonghong Song <yhs@fb.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 bpf 1/2] bpf: verifier: check for packet data access based on target prog
2020-08-21 6:17 ` Yonghong Song
@ 2020-08-21 19:07 ` Udip Pant
2020-08-21 20:53 ` Yonghong Song
0 siblings, 1 reply; 8+ messages in thread
From: Udip Pant @ 2020-08-21 19:07 UTC (permalink / raw)
To: Yonghong Song, Alexei Starovoitov, Martin Lau, Song Liu,
Andrii Nakryiko, David S . Miller
Cc: netdev, bpf, linux-kernel
> On 8/20/20, 11:17 PM, "Yonghong Song" <yhs@fb.com> wrote:
>
>
>
> On 8/20/20 11:13 PM, Yonghong Song wrote:
>>
>>
>> On 8/20/20 5:28 PM, Udip Pant wrote:
>>> While using dynamic program extension (of type BPF_PROG_TYPE_EXT), we
>>> need to check the program type of the target program to grant the read /
>>> write access to the packet data.
>>>
>>> The BPF_PROG_TYPE_EXT type can be used to extend types such as XDP, SKB
>>> and others. Since the BPF_PROG_TYPE_EXT program type on itself is just a
>>> placeholder for those, we need this extended check for those target
>>> programs to actually work while using this option.
>>>
>>> Tested this with a freplace xdp program. Without this patch, the
>>> verifier fails with error 'cannot write into packet'.
>>>
>>> Signed-off-by: Udip Pant <udippant@fb.com>
>>> ---
>>> kernel/bpf/verifier.c | 6 +++++-
>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>> index ef938f17b944..4d7604430994 100644
>>> --- a/kernel/bpf/verifier.c
>>> +++ b/kernel/bpf/verifier.c
>>> @@ -2629,7 +2629,11 @@ static bool may_access_direct_pkt_data(struct
>>> bpf_verifier_env *env,
>>> const struct bpf_call_arg_meta *meta,
>>> enum bpf_access_type t)
>>> {
>>> - switch (env->prog->type) {
>>> + struct bpf_prog *prog = env->prog;
>>> + enum bpf_prog_type prog_type = prog->aux->linked_prog ?
>>> + prog->aux->linked_prog->type : prog->type;
>>
>> I checked the verifier code. There are several places where
>> prog->type is checked and EXT program type will behave differently
>> from the linked program.
>>
>> Maybe abstract the the above logic to one static function like
>>
>> static enum bpf_prog_type resolved_prog_type(struct bpf_prog *prog)
>> {
>> return prog->aux->linked_prog ? prog->aux->linked_prog->type
>> : prog->type;
>> }
>>
Sure.
>> This function can then be used in different places to give the resolved
>> prog type.
>>
>> Besides here checking pkt access permission,
>> another possible places to consider is return value
>> in function check_return_code(). Currently,
>> for EXT program, the result value can be anything. This may need to
>> be enforced. Could you take a look? It could be others as well.
>> You can take a look at verifier.c by searching "prog->type".
>
Yeah there are few other places in the verifier where it decides without resolving for the 'extended' type. But I am not too sure if it makes sense to extend this logic as part of this commit. For example, as you mentioned, in the check_return_code() it explicitly ignores the return type for the EXT prog (kernel/bpf/verifier.c#L7446). Likewise, I noticed similar issue inside the check_ld_abs(), where it checks for may_access_skb(env->prog->type).
I'm happy to extend this logic there as well if deemed appropriate.
> Note that if the EXT program tries to replace a global subprogram,
> then return value cannot be enforced, just as what Patch #2 example shows.
>
>>
>>> +
>>> + switch (prog_type) {
>>> /* Program types only with direct read access go here! */
>>> case BPF_PROG_TYPE_LWT_IN:
>>> case BPF_PROG_TYPE_LWT_OUT:
>>>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 bpf 1/2] bpf: verifier: check for packet data access based on target prog
2020-08-21 19:07 ` Udip Pant
@ 2020-08-21 20:53 ` Yonghong Song
2020-08-21 21:05 ` Alexei Starovoitov
0 siblings, 1 reply; 8+ messages in thread
From: Yonghong Song @ 2020-08-21 20:53 UTC (permalink / raw)
To: Udip Pant, Alexei Starovoitov, Martin Lau, Song Liu,
Andrii Nakryiko, David S . Miller
Cc: netdev, bpf, linux-kernel
On 8/21/20 12:07 PM, Udip Pant wrote:
>
>
> > On 8/20/20, 11:17 PM, "Yonghong Song" <yhs@fb.com> wrote:
>>
>>
>>
>> On 8/20/20 11:13 PM, Yonghong Song wrote:
>>>
>>>
>>> On 8/20/20 5:28 PM, Udip Pant wrote:
>>>> While using dynamic program extension (of type BPF_PROG_TYPE_EXT), we
>>>> need to check the program type of the target program to grant the read /
>>>> write access to the packet data.
>>>>
>>>> The BPF_PROG_TYPE_EXT type can be used to extend types such as XDP, SKB
>>>> and others. Since the BPF_PROG_TYPE_EXT program type on itself is just a
>>>> placeholder for those, we need this extended check for those target
>>>> programs to actually work while using this option.
>>>>
>>>> Tested this with a freplace xdp program. Without this patch, the
>>>> verifier fails with error 'cannot write into packet'.
>>>>
>>>> Signed-off-by: Udip Pant <udippant@fb.com>
>>>> ---
>>>> kernel/bpf/verifier.c | 6 +++++-
>>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>>> index ef938f17b944..4d7604430994 100644
>>>> --- a/kernel/bpf/verifier.c
>>>> +++ b/kernel/bpf/verifier.c
>>>> @@ -2629,7 +2629,11 @@ static bool may_access_direct_pkt_data(struct
>>>> bpf_verifier_env *env,
>>>> const struct bpf_call_arg_meta *meta,
>>>> enum bpf_access_type t)
>>>> {
>>>> - switch (env->prog->type) {
>>>> + struct bpf_prog *prog = env->prog;
>>>> + enum bpf_prog_type prog_type = prog->aux->linked_prog ?
>>>> + prog->aux->linked_prog->type : prog->type;
>>>
>>> I checked the verifier code. There are several places where
>>> prog->type is checked and EXT program type will behave differently
>>> from the linked program.
>>>
>>> Maybe abstract the the above logic to one static function like
>>>
>>> static enum bpf_prog_type resolved_prog_type(struct bpf_prog *prog)
>>> {
>>> return prog->aux->linked_prog ? prog->aux->linked_prog->type
>>> : prog->type;
>>> }
>>>
>
> Sure.
>
>>> This function can then be used in different places to give the resolved
>>> prog type.
>>>
>>> Besides here checking pkt access permission,
>>> another possible places to consider is return value
>>> in function check_return_code(). Currently,
>>> for EXT program, the result value can be anything. This may need to
>>> be enforced. Could you take a look? It could be others as well.
>>> You can take a look at verifier.c by searching "prog->type".
>>
>
> Yeah there are few other places in the verifier where it decides without resolving for the 'extended' type. But I am not too sure if it makes sense to extend this logic as part of this commit. For example, as you mentioned, in the check_return_code() it explicitly ignores the return type for the EXT prog (kernel/bpf/verifier.c#L7446). Likewise, I noticed similar issue inside the check_ld_abs(), where it checks for may_access_skb(env->prog->type).
>
> I'm happy to extend this logic there as well if deemed appropriate.
Thanks. I would like to see the verifier parity between original program
and replace program. That is, if the original program and the replace
program are the same, they should be both either accepted or rejected
by verifier. Yes, this may imply more changes e.g., check_return_code()
or check_ld_abs() than your original patch.
Alexei or Daniel, what is your opinion on this?
>
>> Note that if the EXT program tries to replace a global subprogram,
>> then return value cannot be enforced, just as what Patch #2 example shows.
>>
>>>
>>>> +
>>>> + switch (prog_type) {
>>>> /* Program types only with direct read access go here! */
>>>> case BPF_PROG_TYPE_LWT_IN:
>>>> case BPF_PROG_TYPE_LWT_OUT:
>>>>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 bpf 1/2] bpf: verifier: check for packet data access based on target prog
2020-08-21 20:53 ` Yonghong Song
@ 2020-08-21 21:05 ` Alexei Starovoitov
0 siblings, 0 replies; 8+ messages in thread
From: Alexei Starovoitov @ 2020-08-21 21:05 UTC (permalink / raw)
To: Yonghong Song
Cc: Udip Pant, Alexei Starovoitov, Martin Lau, Song Liu,
Andrii Nakryiko, David S . Miller, netdev, bpf, linux-kernel
On Fri, Aug 21, 2020 at 1:53 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 8/21/20 12:07 PM, Udip Pant wrote:
> >
> >
> > > On 8/20/20, 11:17 PM, "Yonghong Song" <yhs@fb.com> wrote:
> >>
> >>
> >>
> >> On 8/20/20 11:13 PM, Yonghong Song wrote:
> >>>
> >>>
> >>> On 8/20/20 5:28 PM, Udip Pant wrote:
> >>>> While using dynamic program extension (of type BPF_PROG_TYPE_EXT), we
> >>>> need to check the program type of the target program to grant the read /
> >>>> write access to the packet data.
> >>>>
> >>>> The BPF_PROG_TYPE_EXT type can be used to extend types such as XDP, SKB
> >>>> and others. Since the BPF_PROG_TYPE_EXT program type on itself is just a
> >>>> placeholder for those, we need this extended check for those target
> >>>> programs to actually work while using this option.
> >>>>
> >>>> Tested this with a freplace xdp program. Without this patch, the
> >>>> verifier fails with error 'cannot write into packet'.
> >>>>
> >>>> Signed-off-by: Udip Pant <udippant@fb.com>
> >>>> ---
> >>>> kernel/bpf/verifier.c | 6 +++++-
> >>>> 1 file changed, 5 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> >>>> index ef938f17b944..4d7604430994 100644
> >>>> --- a/kernel/bpf/verifier.c
> >>>> +++ b/kernel/bpf/verifier.c
> >>>> @@ -2629,7 +2629,11 @@ static bool may_access_direct_pkt_data(struct
> >>>> bpf_verifier_env *env,
> >>>> const struct bpf_call_arg_meta *meta,
> >>>> enum bpf_access_type t)
> >>>> {
> >>>> - switch (env->prog->type) {
> >>>> + struct bpf_prog *prog = env->prog;
> >>>> + enum bpf_prog_type prog_type = prog->aux->linked_prog ?
> >>>> + prog->aux->linked_prog->type : prog->type;
> >>>
> >>> I checked the verifier code. There are several places where
> >>> prog->type is checked and EXT program type will behave differently
> >>> from the linked program.
> >>>
> >>> Maybe abstract the the above logic to one static function like
> >>>
> >>> static enum bpf_prog_type resolved_prog_type(struct bpf_prog *prog)
> >>> {
> >>> return prog->aux->linked_prog ? prog->aux->linked_prog->type
> >>> : prog->type;
> >>> }
> >>>
> >
> > Sure.
> >
> >>> This function can then be used in different places to give the resolved
> >>> prog type.
> >>>
> >>> Besides here checking pkt access permission,
> >>> another possible places to consider is return value
> >>> in function check_return_code(). Currently,
> >>> for EXT program, the result value can be anything. This may need to
> >>> be enforced. Could you take a look? It could be others as well.
> >>> You can take a look at verifier.c by searching "prog->type".
> >>
> >
> > Yeah there are few other places in the verifier where it decides without resolving for the 'extended' type. But I am not too sure if it makes sense to extend this logic as part of this commit. For example, as you mentioned, in the check_return_code() it explicitly ignores the return type for the EXT prog (kernel/bpf/verifier.c#L7446). Likewise, I noticed similar issue inside the check_ld_abs(), where it checks for may_access_skb(env->prog->type).
> >
> > I'm happy to extend this logic there as well if deemed appropriate.
>
> Thanks. I would like to see the verifier parity between original program
> and replace program. That is, if the original program and the replace
> program are the same, they should be both either accepted or rejected
> by verifier. Yes, this may imply more changes e.g., check_return_code()
> or check_ld_abs() than your original patch.
> Alexei or Daniel, what is your opinion on this?
The set was already marked as 'changes requested' in patchworks.
That's an indication that maintainers agree with the feedback :)
In this particular case it certainly makes sense to address all cases
instead of doing them one at a time.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-08-21 21:05 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-21 0:28 [PATCH v2 bpf 1/2] bpf: verifier: check for packet data access based on target prog Udip Pant
2020-08-21 0:28 ` [PATCH v2 bpf 2/2] selftests/bpf: add test for freplace program with write access Udip Pant
2020-08-21 6:19 ` Yonghong Song
2020-08-21 6:13 ` [PATCH v2 bpf 1/2] bpf: verifier: check for packet data access based on target prog Yonghong Song
2020-08-21 6:17 ` Yonghong Song
2020-08-21 19:07 ` Udip Pant
2020-08-21 20:53 ` Yonghong Song
2020-08-21 21:05 ` Alexei Starovoitov
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).