> [...]
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index 63749ad5ac6b..da575e295d53 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -4493,6 +4493,7 @@ static int check_stack_write_fixed_off(struct bpf_verifier_env *env,
> > > */
> > > if (!env->allow_ptr_leaks &&
> > > is_spilled_reg(&state->stack[spi]) &&
> > > + !is_spilled_scalar_reg(&state->stack[spi]) &&
> > > size != BPF_REG_SIZE) {
> > > verbose(env, "attempt to corrupt spilled pointer on stack\n");
> > > return -EACCES;
> >
> > ack for this part:
> >
> > Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> >
> > but I'm not sure if we need all the *_caps logic for selftests just to
> > test this, tbh. I'll let other decide, though.
>
> Hi Tao, Andrii,
>
> I agree with Andrii that *_caps logic is a bit heavy-handed.
> It works on top of the test_loader.c:drop_capabilities() used for
> unpriv testing, so I suggest that we add only one macro: __caps_unpriv(<set-of-caps>),
> that would enable specified capabilities when testing unpriv.
> E.g. as in the patch at the end of this email (based on Tao's work).
>
> A few additional notes:
> - changes to the verifier.c, to the test_loader.c and actual feature
> test should be split in separate commits;
> - __caps_unpriv as in Tao's or my patches allows to set arbitrary
> capabilities, however restore_capabilities() uses
> cap_enable_effective(requested_caps), which does logical OR
> of current_caps and requested_caps.
> For the purpose of restore_capabilities(), I think
> cap_enable_effective() should be updated with a flag to overwrite
> current_caps with requested_caps. Otherwise it might not undo
> __caps_unpriv() effects in some cases.
> - I changed the test case to avoid BPF_ST assembly instructions
> ('*(u64*)(r10 - 8) = 1') in order to allow compilation by LLVM 16.
>
> Thanks,
> Eduard
Hi Andrii and Eduard,
Thanks for the comments.
Eduard's proposal looks good to me.
If we all agree on it, I can send three separate commits for the bug fix,
the extension on test_loader.c, and the test case for this issue.
Best,
Tao
>
> ---
>
> diff --git a/tools/testing/selftests/bpf/progs/bpf_misc.h b/tools/testing/selftests/bpf/progs/bpf_misc.h
> index fb2f5513e29e..14a79bc76b45 100644
> --- a/tools/testing/selftests/bpf/progs/bpf_misc.h
> +++ b/tools/testing/selftests/bpf/progs/bpf_misc.h
> @@ -2,6 +2,10 @@
> #ifndef __BPF_MISC_H__
> #define __BPF_MISC_H__
>
> +/* Expand a macro and then stringize the expansion */
> +#define QUOTE(str) #str
> +#define EXPAND_QUOTE(str) QUOTE(str)
> +
> /* This set of attributes controls behavior of the
> * test_loader.c:test_loader__run_subtests().
> *
> @@ -72,6 +76,7 @@
> #define __auxiliary __attribute__((btf_decl_tag("comment:test_auxiliary")))
> #define __auxiliary_unpriv __attribute__((btf_decl_tag("comment:test_auxiliary_unpriv")))
> #define __btf_path(path) __attribute__((btf_decl_tag("comment:test_btf_path=" path)))
> +#define __caps_unpriv(caps) __attribute__((btf_decl_tag("comment:test_caps_unpriv="EXPAND_QUOTE(caps))))
>
> /* Convenience macro for use with 'asm volatile' blocks */
> #define __naked __attribute__((naked))
> diff --git a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
> index 85e48069c9e6..a043514da4f0 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
> @@ -5,6 +5,7 @@
> #include <bpf/bpf_helpers.h>
> #include "bpf_misc.h"
> #include <../../../tools/include/linux/filter.h>
> +#include "../cap_helpers.h"
>
> struct {
> __uint(type, BPF_MAP_TYPE_RINGBUF);
> @@ -1244,4 +1245,19 @@ __naked void old_stack_misc_vs_cur_ctx_ptr(void)
> : __clobber_all);
> }
>
> +/* 32-bit scalars should be able to overwrite 64-bit scalar spilled slots */
> +SEC("socket")
> +__log_level(2) __success __caps_unpriv(CAP_BPF)
> +__naked void spill_32bit_onto_64bit_slot(void)
> +{
> + asm volatile(" \
> + r0 = 0; \
> + *(u64*)(r10 - 8) = r0; \
> + *(u32*)(r10 - 8) = r0; \
> + exit; \
> +" :
> + :
> + : __clobber_all);
> +}
> +
> char _license[] SEC("license") = "GPL";
> diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c
> index 524c38e9cde4..ad79b5948440 100644
> --- a/tools/testing/selftests/bpf/test_loader.c
> +++ b/tools/testing/selftests/bpf/test_loader.c
> @@ -27,6 +27,7 @@
> #define TEST_TAG_RETVAL_PFX_UNPRIV "comment:test_retval_unpriv="
> #define TEST_TAG_AUXILIARY "comment:test_auxiliary"
> #define TEST_TAG_AUXILIARY_UNPRIV "comment:test_auxiliary_unpriv"
> +#define TEST_TAG_CAPS_UNPRIV "comment:test_caps_unpriv="
> #define TEST_BTF_PATH "comment:test_btf_path="
>
> /* Warning: duplicated in bpf_misc.h */
> @@ -53,6 +54,7 @@ struct test_subspec {
> size_t expect_msg_cnt;
> int retval;
> bool execute;
> + __u64 caps;
> };
>
> struct test_spec {
> @@ -133,6 +135,33 @@ static int parse_int(const char *str, int *val, const char *name)
> return 0;
> }
>
> +static int parse_caps(const char *str, __u64 *val, const char *name)
> +{
> + int cap_flag = 0;
> + char *token = NULL, *saveptr = NULL;
> +
> + char *str_cpy = strdup(str);
> + if (str_cpy == NULL) {
> + PRINT_FAIL("Memory allocation failed\n");
> + return -EINVAL;
> + }
> +
> + token = strtok_r(str_cpy, "|", &saveptr);
> + while (token != NULL) {
> + errno = 0;
> + cap_flag = strtol(token, NULL, 10);
> + if (errno) {
> + PRINT_FAIL("failed to parse caps %s\n", name);
> + return -EINVAL;
> + }
> + *val |= (1ULL << cap_flag);
> + token = strtok_r(NULL, "|", &saveptr);
> + }
> +
> + free(str_cpy);
> + return 0;
> +}
> +
> static int parse_retval(const char *str, int *val, const char *name)
> {
> struct {
> @@ -258,6 +287,12 @@ static int parse_test_spec(struct test_loader *tester,
> spec->mode_mask |= UNPRIV;
> spec->unpriv.execute = true;
> has_unpriv_retval = true;
> + } else if (str_has_pfx(s, TEST_TAG_CAPS_UNPRIV)) {
> + val = s + sizeof(TEST_TAG_CAPS_UNPRIV) - 1;
> + err = parse_caps(val, &spec->unpriv.caps, "test caps");
> + if (err)
> + goto cleanup;
> + spec->mode_mask |= UNPRIV;
> } else if (str_has_pfx(s, TEST_TAG_LOG_LEVEL_PFX)) {
> val = s + sizeof(TEST_TAG_LOG_LEVEL_PFX) - 1;
> err = parse_int(val, &spec->log_level, "test log level");
> @@ -575,6 +610,7 @@ void run_subtest(struct test_loader *tester,
> return;
>
> if (unpriv) {
> + fprintf(stderr, "can_execute_unpriv: %d\n", can_execute_unpriv(tester, spec));
> if (!can_execute_unpriv(tester, spec)) {
> test__skip();
> test__end_subtest();
> @@ -584,6 +620,13 @@ void run_subtest(struct test_loader *tester,
> test__end_subtest();
> return;
> }
> + if (subspec->caps) {
> + err = cap_enable_effective(subspec->caps, NULL);
> + if (err) {
> + PRINT_FAIL("failed to set capabilities: %i, %s\n", err, strerror(err));
> + goto subtest_cleanup;
> + }
> + }
> }
>
> /* Implicitly reset to NULL if next test case doesn't specify */
