| Message ID | 20210325154034.85346-2-toke@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf,1/2] bpf: enforce that struct_ops programs be GPL-only | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 2 maintainers not CCed: linux-kselftest@vger.kernel.org shuah@kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: line length of 81 exceeds 80 columns WARNING: line length of 82 exceeds 80 columns WARNING: line length of 83 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On Thu, Mar 25, 2021 at 04:40:34PM +0100, Toke Høiland-Jørgensen wrote: > This adds a selftest to check that the verifier rejects a TCP CC struct_ops > with a non-GPL license. To save having to add a whole new BPF object just > for this, reuse the dctcp CC, but rewrite the license field before loading. > > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> > --- > .../selftests/bpf/prog_tests/bpf_tcp_ca.c | 31 +++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c > index 37c5494a0381..613cf8a00b22 100644 > --- a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c > +++ b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c > @@ -227,10 +227,41 @@ static void test_dctcp(void) > bpf_dctcp__destroy(dctcp_skel); > } > > +static void test_invalid_license(void) > +{ > + /* We want to check that the verifier refuses to load a non-GPL TCP CC. > + * Rather than create a whole new file+skeleton, just reuse an existing > + * object and rewrite the license in memory after loading. Sine libbpf > + * doesn't expose this, we define a struct that includes the first couple > + * of internal fields for struct bpf_object so we can overwrite the right > + * bits. Yes, this is a bit of a hack, but it makes the test a lot simpler. > + */ > + struct bpf_object_fragment { > + char name[BPF_OBJ_NAME_LEN]; > + char license[64]; > + } *obj; It is fragile. A new bpf_nogpltcp.c should be created and it does not have to be a full tcp-cc. A very minimal implementation with only .init. Something like this (uncompiled code): char _license[] SEC("license") = "X"; void BPF_STRUCT_OPS(nogpltcp_init, struct sock *sk) { } SEC(".struct_ops") struct tcp_congestion_ops bpf_nogpltcp = { .init = (void *)nogpltcp_init, .name = "bpf_nogpltcp", }; libbpf_set_print() can also be used to look for the the verifier log "struct ops programs must have a GPL compatible license".
Martin KaFai Lau <kafai@fb.com> writes: > On Thu, Mar 25, 2021 at 04:40:34PM +0100, Toke Høiland-Jørgensen wrote: >> This adds a selftest to check that the verifier rejects a TCP CC struct_ops >> with a non-GPL license. To save having to add a whole new BPF object just >> for this, reuse the dctcp CC, but rewrite the license field before loading. >> >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> >> --- >> .../selftests/bpf/prog_tests/bpf_tcp_ca.c | 31 +++++++++++++++++++ >> 1 file changed, 31 insertions(+) >> >> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c >> index 37c5494a0381..613cf8a00b22 100644 >> --- a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c >> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c >> @@ -227,10 +227,41 @@ static void test_dctcp(void) >> bpf_dctcp__destroy(dctcp_skel); >> } >> >> +static void test_invalid_license(void) >> +{ >> + /* We want to check that the verifier refuses to load a non-GPL TCP CC. >> + * Rather than create a whole new file+skeleton, just reuse an existing >> + * object and rewrite the license in memory after loading. Sine libbpf >> + * doesn't expose this, we define a struct that includes the first couple >> + * of internal fields for struct bpf_object so we can overwrite the right >> + * bits. Yes, this is a bit of a hack, but it makes the test a lot simpler. >> + */ >> + struct bpf_object_fragment { >> + char name[BPF_OBJ_NAME_LEN]; >> + char license[64]; >> + } *obj; > It is fragile. A new bpf_nogpltcp.c should be created and it does > not have to be a full tcp-cc. A very minimal implementation with > only .init. Something like this (uncompiled code): > > char _license[] SEC("license") = "X"; > > void BPF_STRUCT_OPS(nogpltcp_init, struct sock *sk) > { > } > > SEC(".struct_ops") > struct tcp_congestion_ops bpf_nogpltcp = { > .init = (void *)nogpltcp_init, > .name = "bpf_nogpltcp", > }; > > libbpf_set_print() can also be used to look for the > the verifier log "struct ops programs must have a GPL compatible license". Ah, thanks for the pointers! Will fix this up as well... -Toke
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c index 37c5494a0381..613cf8a00b22 100644 --- a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c +++ b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c @@ -227,10 +227,41 @@ static void test_dctcp(void) bpf_dctcp__destroy(dctcp_skel); } +static void test_invalid_license(void) +{ + /* We want to check that the verifier refuses to load a non-GPL TCP CC. + * Rather than create a whole new file+skeleton, just reuse an existing + * object and rewrite the license in memory after loading. Sine libbpf + * doesn't expose this, we define a struct that includes the first couple + * of internal fields for struct bpf_object so we can overwrite the right + * bits. Yes, this is a bit of a hack, but it makes the test a lot simpler. + */ + struct bpf_object_fragment { + char name[BPF_OBJ_NAME_LEN]; + char license[64]; + } *obj; + struct bpf_dctcp *skel; + int err; + + skel = bpf_dctcp__open(); + if (CHECK(!skel, "bpf_dctcp__open", "failed\n")) + return; + + obj = (void *)skel->obj; + obj->license[0] = 'X'; // turns 'GPL' into 'XPL' which will fail the check + + err = bpf_dctcp__load(skel); + CHECK(err != -LIBBPF_ERRNO__VERIFY, "bpf_dctcp__load", "err:%d\n", err); + + bpf_dctcp__destroy(skel); +} + void test_bpf_tcp_ca(void) { if (test__start_subtest("dctcp")) test_dctcp(); if (test__start_subtest("cubic")) test_cubic(); + if (test__start_subtest("invalid_license")) + test_invalid_license(); }
This adds a selftest to check that the verifier rejects a TCP CC struct_ops with a non-GPL license. To save having to add a whole new BPF object just for this, reuse the dctcp CC, but rewrite the license field before loading. Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> --- .../selftests/bpf/prog_tests/bpf_tcp_ca.c | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+)