Message ID | 20230427200409.1785263-3-sdf@google.com (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | BPF |
Headers | show |
Series | bpf: Don't EFAULT for {g,s}setsockopt with wrong optlen | expand |
On 4/27/23 1:04 PM, Stanislav Fomichev wrote: > Instead of assuming EFAULT, let's assume the BPF program's > output is ignored. > > Remove "getsockopt: deny arbitrary ctx->retval" because it > was actually testing optlen. We have separate set of tests > for retval. > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > --- > .../selftests/bpf/prog_tests/sockopt.c | 80 +++++++++++++++++-- > 1 file changed, 74 insertions(+), 6 deletions(-) > > diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt.c b/tools/testing/selftests/bpf/prog_tests/sockopt.c > index aa4debf62fc6..8dad30ce910e 100644 > --- a/tools/testing/selftests/bpf/prog_tests/sockopt.c > +++ b/tools/testing/selftests/bpf/prog_tests/sockopt.c > @@ -273,10 +273,30 @@ static struct sockopt_test { > .error = EFAULT_GETSOCKOPT, > }, > { > - .descr = "getsockopt: deny arbitrary ctx->retval", > + .descr = "getsockopt: ignore >PAGE_SIZE optlen", > .insns = { > - /* ctx->retval = 123 */ > - BPF_MOV64_IMM(BPF_REG_0, 123), > + /* write 0xFF to the first optval byte */ > + > + /* r6 = ctx->optval */ > + BPF_LDX_MEM(BPF_DW, BPF_REG_6, BPF_REG_1, > + offsetof(struct bpf_sockopt, optval)), > + /* r2 = ctx->optval */ > + BPF_MOV64_REG(BPF_REG_2, BPF_REG_6), > + /* r6 = ctx->optval + 1 */ > + BPF_ALU64_IMM(BPF_ADD, BPF_REG_6, 1), > + > + /* r7 = ctx->optval_end */ > + BPF_LDX_MEM(BPF_DW, BPF_REG_7, BPF_REG_1, > + offsetof(struct bpf_sockopt, optval_end)), > + > + /* if (ctx->optval + 1 <= ctx->optval_end) { */ > + BPF_JMP_REG(BPF_JGT, BPF_REG_6, BPF_REG_7, 1), > + /* ctx->optval[0] = 0xF0 */ > + BPF_ST_MEM(BPF_B, BPF_REG_2, 0, 0xFF), > + /* } */ > + > + /* ctx->retval = 0 */ > + BPF_MOV64_IMM(BPF_REG_0, 0), > BPF_STX_MEM(BPF_W, BPF_REG_1, BPF_REG_0, > offsetof(struct bpf_sockopt, retval)), > > @@ -287,9 +307,10 @@ static struct sockopt_test { > .attach_type = BPF_CGROUP_GETSOCKOPT, > .expected_attach_type = BPF_CGROUP_GETSOCKOPT, > > - .get_optlen = 64, > - > - .error = EFAULT_GETSOCKOPT, > + .get_level = 1234, > + .get_optname = 5678, > + .get_optval = {}, /* the changes are ignored */ > + .get_optlen = 4096 + 1, The patchset looks good. Thanks for taking care of it. One question, is it safe to the assume 4096 page size for all platforms in the selftests?
On Fri, Apr 28, 2023 at 4:57 PM Martin KaFai Lau <martin.lau@linux.dev> wrote: > > On 4/27/23 1:04 PM, Stanislav Fomichev wrote: > > Instead of assuming EFAULT, let's assume the BPF program's > > output is ignored. > > > > Remove "getsockopt: deny arbitrary ctx->retval" because it > > was actually testing optlen. We have separate set of tests > > for retval. > > > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > > --- > > .../selftests/bpf/prog_tests/sockopt.c | 80 +++++++++++++++++-- > > 1 file changed, 74 insertions(+), 6 deletions(-) > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt.c b/tools/testing/selftests/bpf/prog_tests/sockopt.c > > index aa4debf62fc6..8dad30ce910e 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/sockopt.c > > +++ b/tools/testing/selftests/bpf/prog_tests/sockopt.c > > @@ -273,10 +273,30 @@ static struct sockopt_test { > > .error = EFAULT_GETSOCKOPT, > > }, > > { > > - .descr = "getsockopt: deny arbitrary ctx->retval", > > + .descr = "getsockopt: ignore >PAGE_SIZE optlen", > > .insns = { > > - /* ctx->retval = 123 */ > > - BPF_MOV64_IMM(BPF_REG_0, 123), > > + /* write 0xFF to the first optval byte */ > > + > > + /* r6 = ctx->optval */ > > + BPF_LDX_MEM(BPF_DW, BPF_REG_6, BPF_REG_1, > > + offsetof(struct bpf_sockopt, optval)), > > + /* r2 = ctx->optval */ > > + BPF_MOV64_REG(BPF_REG_2, BPF_REG_6), > > + /* r6 = ctx->optval + 1 */ > > + BPF_ALU64_IMM(BPF_ADD, BPF_REG_6, 1), > > + > > + /* r7 = ctx->optval_end */ > > + BPF_LDX_MEM(BPF_DW, BPF_REG_7, BPF_REG_1, > > + offsetof(struct bpf_sockopt, optval_end)), > > + > > + /* if (ctx->optval + 1 <= ctx->optval_end) { */ > > + BPF_JMP_REG(BPF_JGT, BPF_REG_6, BPF_REG_7, 1), > > + /* ctx->optval[0] = 0xF0 */ > > + BPF_ST_MEM(BPF_B, BPF_REG_2, 0, 0xFF), > > + /* } */ > > + > > + /* ctx->retval = 0 */ > > + BPF_MOV64_IMM(BPF_REG_0, 0), > > BPF_STX_MEM(BPF_W, BPF_REG_1, BPF_REG_0, > > offsetof(struct bpf_sockopt, retval)), > > > > @@ -287,9 +307,10 @@ static struct sockopt_test { > > .attach_type = BPF_CGROUP_GETSOCKOPT, > > .expected_attach_type = BPF_CGROUP_GETSOCKOPT, > > > > - .get_optlen = 64, > > - > > - .error = EFAULT_GETSOCKOPT, > > + .get_level = 1234, > > + .get_optname = 5678, > > + .get_optval = {}, /* the changes are ignored */ > > + .get_optlen = 4096 + 1, > > The patchset looks good. Thanks for taking care of it. > > One question, is it safe to the assume 4096 page size for all platforms in the > selftests? Good question; let me respin with sysconf() just to be safe..
On Fri, Apr 28, 2023 at 4:59 PM Stanislav Fomichev <sdf@google.com> wrote: > > On Fri, Apr 28, 2023 at 4:57 PM Martin KaFai Lau <martin.lau@linux.dev> wrote: > > > > On 4/27/23 1:04 PM, Stanislav Fomichev wrote: > > > Instead of assuming EFAULT, let's assume the BPF program's > > > output is ignored. > > > > > > Remove "getsockopt: deny arbitrary ctx->retval" because it > > > was actually testing optlen. We have separate set of tests > > > for retval. > > > > > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > > > --- > > > .../selftests/bpf/prog_tests/sockopt.c | 80 +++++++++++++++++-- > > > 1 file changed, 74 insertions(+), 6 deletions(-) > > > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt.c b/tools/testing/selftests/bpf/prog_tests/sockopt.c > > > index aa4debf62fc6..8dad30ce910e 100644 > > > --- a/tools/testing/selftests/bpf/prog_tests/sockopt.c > > > +++ b/tools/testing/selftests/bpf/prog_tests/sockopt.c > > > @@ -273,10 +273,30 @@ static struct sockopt_test { > > > .error = EFAULT_GETSOCKOPT, > > > }, > > > { > > > - .descr = "getsockopt: deny arbitrary ctx->retval", > > > + .descr = "getsockopt: ignore >PAGE_SIZE optlen", > > > .insns = { > > > - /* ctx->retval = 123 */ > > > - BPF_MOV64_IMM(BPF_REG_0, 123), > > > + /* write 0xFF to the first optval byte */ > > > + > > > + /* r6 = ctx->optval */ > > > + BPF_LDX_MEM(BPF_DW, BPF_REG_6, BPF_REG_1, > > > + offsetof(struct bpf_sockopt, optval)), > > > + /* r2 = ctx->optval */ > > > + BPF_MOV64_REG(BPF_REG_2, BPF_REG_6), > > > + /* r6 = ctx->optval + 1 */ > > > + BPF_ALU64_IMM(BPF_ADD, BPF_REG_6, 1), > > > + > > > + /* r7 = ctx->optval_end */ > > > + BPF_LDX_MEM(BPF_DW, BPF_REG_7, BPF_REG_1, > > > + offsetof(struct bpf_sockopt, optval_end)), > > > + > > > + /* if (ctx->optval + 1 <= ctx->optval_end) { */ > > > + BPF_JMP_REG(BPF_JGT, BPF_REG_6, BPF_REG_7, 1), > > > + /* ctx->optval[0] = 0xF0 */ > > > + BPF_ST_MEM(BPF_B, BPF_REG_2, 0, 0xFF), > > > + /* } */ > > > + > > > + /* ctx->retval = 0 */ > > > + BPF_MOV64_IMM(BPF_REG_0, 0), > > > BPF_STX_MEM(BPF_W, BPF_REG_1, BPF_REG_0, > > > offsetof(struct bpf_sockopt, retval)), > > > > > > @@ -287,9 +307,10 @@ static struct sockopt_test { > > > .attach_type = BPF_CGROUP_GETSOCKOPT, > > > .expected_attach_type = BPF_CGROUP_GETSOCKOPT, > > > > > > - .get_optlen = 64, > > > - > > > - .error = EFAULT_GETSOCKOPT, > > > + .get_level = 1234, > > > + .get_optname = 5678, > > > + .get_optval = {}, /* the changes are ignored */ > > > + .get_optlen = 4096 + 1, > > > > The patchset looks good. Thanks for taking care of it. > > > > One question, is it safe to the assume 4096 page size for all platforms in the > > selftests? > > Good question; let me respin with sysconf() just to be safe.. Argh, the compiler yells at me: error: initializer element is not a compile-time constant I guess I'm just gonna do #define PAGE_SIZE 4096 and if we do hit some problems on the other archs, I'll ifdef it in one place.
On 4/28/23 5:32 PM, Stanislav Fomichev wrote: > On Fri, Apr 28, 2023 at 4:59 PM Stanislav Fomichev <sdf@google.com> wrote: >> >> On Fri, Apr 28, 2023 at 4:57 PM Martin KaFai Lau <martin.lau@linux.dev> wrote: >>> >>> On 4/27/23 1:04 PM, Stanislav Fomichev wrote: >>>> Instead of assuming EFAULT, let's assume the BPF program's >>>> output is ignored. >>>> >>>> Remove "getsockopt: deny arbitrary ctx->retval" because it >>>> was actually testing optlen. We have separate set of tests >>>> for retval. >>>> >>>> Signed-off-by: Stanislav Fomichev <sdf@google.com> >>>> --- >>>> .../selftests/bpf/prog_tests/sockopt.c | 80 +++++++++++++++++-- >>>> 1 file changed, 74 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt.c b/tools/testing/selftests/bpf/prog_tests/sockopt.c >>>> index aa4debf62fc6..8dad30ce910e 100644 >>>> --- a/tools/testing/selftests/bpf/prog_tests/sockopt.c >>>> +++ b/tools/testing/selftests/bpf/prog_tests/sockopt.c >>>> @@ -273,10 +273,30 @@ static struct sockopt_test { >>>> .error = EFAULT_GETSOCKOPT, >>>> }, >>>> { >>>> - .descr = "getsockopt: deny arbitrary ctx->retval", >>>> + .descr = "getsockopt: ignore >PAGE_SIZE optlen", >>>> .insns = { >>>> - /* ctx->retval = 123 */ >>>> - BPF_MOV64_IMM(BPF_REG_0, 123), >>>> + /* write 0xFF to the first optval byte */ >>>> + >>>> + /* r6 = ctx->optval */ >>>> + BPF_LDX_MEM(BPF_DW, BPF_REG_6, BPF_REG_1, >>>> + offsetof(struct bpf_sockopt, optval)), >>>> + /* r2 = ctx->optval */ >>>> + BPF_MOV64_REG(BPF_REG_2, BPF_REG_6), >>>> + /* r6 = ctx->optval + 1 */ >>>> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_6, 1), >>>> + >>>> + /* r7 = ctx->optval_end */ >>>> + BPF_LDX_MEM(BPF_DW, BPF_REG_7, BPF_REG_1, >>>> + offsetof(struct bpf_sockopt, optval_end)), >>>> + >>>> + /* if (ctx->optval + 1 <= ctx->optval_end) { */ >>>> + BPF_JMP_REG(BPF_JGT, BPF_REG_6, BPF_REG_7, 1), >>>> + /* ctx->optval[0] = 0xF0 */ >>>> + BPF_ST_MEM(BPF_B, BPF_REG_2, 0, 0xFF), >>>> + /* } */ >>>> + >>>> + /* ctx->retval = 0 */ >>>> + BPF_MOV64_IMM(BPF_REG_0, 0), >>>> BPF_STX_MEM(BPF_W, BPF_REG_1, BPF_REG_0, >>>> offsetof(struct bpf_sockopt, retval)), >>>> >>>> @@ -287,9 +307,10 @@ static struct sockopt_test { >>>> .attach_type = BPF_CGROUP_GETSOCKOPT, >>>> .expected_attach_type = BPF_CGROUP_GETSOCKOPT, >>>> >>>> - .get_optlen = 64, >>>> - >>>> - .error = EFAULT_GETSOCKOPT, >>>> + .get_level = 1234, >>>> + .get_optname = 5678, >>>> + .get_optval = {}, /* the changes are ignored */ >>>> + .get_optlen = 4096 + 1, >>> >>> The patchset looks good. Thanks for taking care of it. >>> >>> One question, is it safe to the assume 4096 page size for all platforms in the >>> selftests? >> >> Good question; let me respin with sysconf() just to be safe.. > > Argh, the compiler yells at me: > error: initializer element is not a compile-time constant > > I guess I'm just gonna do #define PAGE_SIZE 4096 and if we do hit some > problems on the other archs, I'll ifdef it in one place. or run_test() can reinit optlen to sysconf_page_size + 1 if optlen == 4097.
On Fri, Apr 28, 2023 at 5:44 PM Martin KaFai Lau <martin.lau@linux.dev> wrote: > > On 4/28/23 5:32 PM, Stanislav Fomichev wrote: > > On Fri, Apr 28, 2023 at 4:59 PM Stanislav Fomichev <sdf@google.com> wrote: > >> > >> On Fri, Apr 28, 2023 at 4:57 PM Martin KaFai Lau <martin.lau@linux.dev> wrote: > >>> > >>> On 4/27/23 1:04 PM, Stanislav Fomichev wrote: > >>>> Instead of assuming EFAULT, let's assume the BPF program's > >>>> output is ignored. > >>>> > >>>> Remove "getsockopt: deny arbitrary ctx->retval" because it > >>>> was actually testing optlen. We have separate set of tests > >>>> for retval. > >>>> > >>>> Signed-off-by: Stanislav Fomichev <sdf@google.com> > >>>> --- > >>>> .../selftests/bpf/prog_tests/sockopt.c | 80 +++++++++++++++++-- > >>>> 1 file changed, 74 insertions(+), 6 deletions(-) > >>>> > >>>> diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt.c b/tools/testing/selftests/bpf/prog_tests/sockopt.c > >>>> index aa4debf62fc6..8dad30ce910e 100644 > >>>> --- a/tools/testing/selftests/bpf/prog_tests/sockopt.c > >>>> +++ b/tools/testing/selftests/bpf/prog_tests/sockopt.c > >>>> @@ -273,10 +273,30 @@ static struct sockopt_test { > >>>> .error = EFAULT_GETSOCKOPT, > >>>> }, > >>>> { > >>>> - .descr = "getsockopt: deny arbitrary ctx->retval", > >>>> + .descr = "getsockopt: ignore >PAGE_SIZE optlen", > >>>> .insns = { > >>>> - /* ctx->retval = 123 */ > >>>> - BPF_MOV64_IMM(BPF_REG_0, 123), > >>>> + /* write 0xFF to the first optval byte */ > >>>> + > >>>> + /* r6 = ctx->optval */ > >>>> + BPF_LDX_MEM(BPF_DW, BPF_REG_6, BPF_REG_1, > >>>> + offsetof(struct bpf_sockopt, optval)), > >>>> + /* r2 = ctx->optval */ > >>>> + BPF_MOV64_REG(BPF_REG_2, BPF_REG_6), > >>>> + /* r6 = ctx->optval + 1 */ > >>>> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_6, 1), > >>>> + > >>>> + /* r7 = ctx->optval_end */ > >>>> + BPF_LDX_MEM(BPF_DW, BPF_REG_7, BPF_REG_1, > >>>> + offsetof(struct bpf_sockopt, optval_end)), > >>>> + > >>>> + /* if (ctx->optval + 1 <= ctx->optval_end) { */ > >>>> + BPF_JMP_REG(BPF_JGT, BPF_REG_6, BPF_REG_7, 1), > >>>> + /* ctx->optval[0] = 0xF0 */ > >>>> + BPF_ST_MEM(BPF_B, BPF_REG_2, 0, 0xFF), > >>>> + /* } */ > >>>> + > >>>> + /* ctx->retval = 0 */ > >>>> + BPF_MOV64_IMM(BPF_REG_0, 0), > >>>> BPF_STX_MEM(BPF_W, BPF_REG_1, BPF_REG_0, > >>>> offsetof(struct bpf_sockopt, retval)), > >>>> > >>>> @@ -287,9 +307,10 @@ static struct sockopt_test { > >>>> .attach_type = BPF_CGROUP_GETSOCKOPT, > >>>> .expected_attach_type = BPF_CGROUP_GETSOCKOPT, > >>>> > >>>> - .get_optlen = 64, > >>>> - > >>>> - .error = EFAULT_GETSOCKOPT, > >>>> + .get_level = 1234, > >>>> + .get_optname = 5678, > >>>> + .get_optval = {}, /* the changes are ignored */ > >>>> + .get_optlen = 4096 + 1, > >>> > >>> The patchset looks good. Thanks for taking care of it. > >>> > >>> One question, is it safe to the assume 4096 page size for all platforms in the > >>> selftests? > >> > >> Good question; let me respin with sysconf() just to be safe.. > > > > Argh, the compiler yells at me: > > error: initializer element is not a compile-time constant > > > > I guess I'm just gonna do #define PAGE_SIZE 4096 and if we do hit some > > problems on the other archs, I'll ifdef it in one place. > > or run_test() can reinit optlen to sysconf_page_size + 1 if optlen == 4097. Maybe I can do something like the following? if (test->set_optlen >= PAGE_SIZE) { int num_pages = test->set_optlen / PAGE_SIZE; int remainder = test->set_optlen % PAGE_SIZE; test->set_optlen = num_pages * sysconf(_SC_PAGESIZE) + remainder; } More verbose, but less magical than depending on 4097. For the BPF side, I can probably pass proper value via bss..
On 5/1/23 10:22 AM, Stanislav Fomichev wrote: > On Fri, Apr 28, 2023 at 5:44 PM Martin KaFai Lau <martin.lau@linux.dev> wrote: >> >> On 4/28/23 5:32 PM, Stanislav Fomichev wrote: >>> On Fri, Apr 28, 2023 at 4:59 PM Stanislav Fomichev <sdf@google.com> wrote: >>>> >>>> On Fri, Apr 28, 2023 at 4:57 PM Martin KaFai Lau <martin.lau@linux.dev> wrote: >>>>> >>>>> On 4/27/23 1:04 PM, Stanislav Fomichev wrote: >>>>>> Instead of assuming EFAULT, let's assume the BPF program's >>>>>> output is ignored. >>>>>> >>>>>> Remove "getsockopt: deny arbitrary ctx->retval" because it >>>>>> was actually testing optlen. We have separate set of tests >>>>>> for retval. >>>>>> >>>>>> Signed-off-by: Stanislav Fomichev <sdf@google.com> >>>>>> --- >>>>>> .../selftests/bpf/prog_tests/sockopt.c | 80 +++++++++++++++++-- >>>>>> 1 file changed, 74 insertions(+), 6 deletions(-) >>>>>> >>>>>> diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt.c b/tools/testing/selftests/bpf/prog_tests/sockopt.c >>>>>> index aa4debf62fc6..8dad30ce910e 100644 >>>>>> --- a/tools/testing/selftests/bpf/prog_tests/sockopt.c >>>>>> +++ b/tools/testing/selftests/bpf/prog_tests/sockopt.c >>>>>> @@ -273,10 +273,30 @@ static struct sockopt_test { >>>>>> .error = EFAULT_GETSOCKOPT, >>>>>> }, >>>>>> { >>>>>> - .descr = "getsockopt: deny arbitrary ctx->retval", >>>>>> + .descr = "getsockopt: ignore >PAGE_SIZE optlen", >>>>>> .insns = { >>>>>> - /* ctx->retval = 123 */ >>>>>> - BPF_MOV64_IMM(BPF_REG_0, 123), >>>>>> + /* write 0xFF to the first optval byte */ >>>>>> + >>>>>> + /* r6 = ctx->optval */ >>>>>> + BPF_LDX_MEM(BPF_DW, BPF_REG_6, BPF_REG_1, >>>>>> + offsetof(struct bpf_sockopt, optval)), >>>>>> + /* r2 = ctx->optval */ >>>>>> + BPF_MOV64_REG(BPF_REG_2, BPF_REG_6), >>>>>> + /* r6 = ctx->optval + 1 */ >>>>>> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_6, 1), >>>>>> + >>>>>> + /* r7 = ctx->optval_end */ >>>>>> + BPF_LDX_MEM(BPF_DW, BPF_REG_7, BPF_REG_1, >>>>>> + offsetof(struct bpf_sockopt, optval_end)), >>>>>> + >>>>>> + /* if (ctx->optval + 1 <= ctx->optval_end) { */ >>>>>> + BPF_JMP_REG(BPF_JGT, BPF_REG_6, BPF_REG_7, 1), >>>>>> + /* ctx->optval[0] = 0xF0 */ >>>>>> + BPF_ST_MEM(BPF_B, BPF_REG_2, 0, 0xFF), >>>>>> + /* } */ >>>>>> + >>>>>> + /* ctx->retval = 0 */ >>>>>> + BPF_MOV64_IMM(BPF_REG_0, 0), >>>>>> BPF_STX_MEM(BPF_W, BPF_REG_1, BPF_REG_0, >>>>>> offsetof(struct bpf_sockopt, retval)), >>>>>> >>>>>> @@ -287,9 +307,10 @@ static struct sockopt_test { >>>>>> .attach_type = BPF_CGROUP_GETSOCKOPT, >>>>>> .expected_attach_type = BPF_CGROUP_GETSOCKOPT, >>>>>> >>>>>> - .get_optlen = 64, >>>>>> - >>>>>> - .error = EFAULT_GETSOCKOPT, >>>>>> + .get_level = 1234, >>>>>> + .get_optname = 5678, >>>>>> + .get_optval = {}, /* the changes are ignored */ >>>>>> + .get_optlen = 4096 + 1, >>>>> >>>>> The patchset looks good. Thanks for taking care of it. >>>>> >>>>> One question, is it safe to the assume 4096 page size for all platforms in the >>>>> selftests? >>>> >>>> Good question; let me respin with sysconf() just to be safe.. >>> >>> Argh, the compiler yells at me: >>> error: initializer element is not a compile-time constant >>> >>> I guess I'm just gonna do #define PAGE_SIZE 4096 and if we do hit some >>> problems on the other archs, I'll ifdef it in one place. >> >> or run_test() can reinit optlen to sysconf_page_size + 1 if optlen == 4097. > > Maybe I can do something like the following? > > if (test->set_optlen >= PAGE_SIZE) { > int num_pages = test->set_optlen / PAGE_SIZE; > int remainder = test->set_optlen % PAGE_SIZE; > > test->set_optlen = num_pages * > sysconf(_SC_PAGESIZE) + remainder; > } > > More verbose, but less magical than depending on 4097. LGTM. > For the BPF side, I can probably pass proper value via bss.. Make sense also.
diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt.c b/tools/testing/selftests/bpf/prog_tests/sockopt.c index aa4debf62fc6..8dad30ce910e 100644 --- a/tools/testing/selftests/bpf/prog_tests/sockopt.c +++ b/tools/testing/selftests/bpf/prog_tests/sockopt.c @@ -273,10 +273,30 @@ static struct sockopt_test { .error = EFAULT_GETSOCKOPT, }, { - .descr = "getsockopt: deny arbitrary ctx->retval", + .descr = "getsockopt: ignore >PAGE_SIZE optlen", .insns = { - /* ctx->retval = 123 */ - BPF_MOV64_IMM(BPF_REG_0, 123), + /* write 0xFF to the first optval byte */ + + /* r6 = ctx->optval */ + BPF_LDX_MEM(BPF_DW, BPF_REG_6, BPF_REG_1, + offsetof(struct bpf_sockopt, optval)), + /* r2 = ctx->optval */ + BPF_MOV64_REG(BPF_REG_2, BPF_REG_6), + /* r6 = ctx->optval + 1 */ + BPF_ALU64_IMM(BPF_ADD, BPF_REG_6, 1), + + /* r7 = ctx->optval_end */ + BPF_LDX_MEM(BPF_DW, BPF_REG_7, BPF_REG_1, + offsetof(struct bpf_sockopt, optval_end)), + + /* if (ctx->optval + 1 <= ctx->optval_end) { */ + BPF_JMP_REG(BPF_JGT, BPF_REG_6, BPF_REG_7, 1), + /* ctx->optval[0] = 0xF0 */ + BPF_ST_MEM(BPF_B, BPF_REG_2, 0, 0xFF), + /* } */ + + /* ctx->retval = 0 */ + BPF_MOV64_IMM(BPF_REG_0, 0), BPF_STX_MEM(BPF_W, BPF_REG_1, BPF_REG_0, offsetof(struct bpf_sockopt, retval)), @@ -287,9 +307,10 @@ static struct sockopt_test { .attach_type = BPF_CGROUP_GETSOCKOPT, .expected_attach_type = BPF_CGROUP_GETSOCKOPT, - .get_optlen = 64, - - .error = EFAULT_GETSOCKOPT, + .get_level = 1234, + .get_optname = 5678, + .get_optval = {}, /* the changes are ignored */ + .get_optlen = 4096 + 1, }, { .descr = "getsockopt: support smaller ctx->optlen", @@ -648,6 +669,49 @@ static struct sockopt_test { .error = EFAULT_SETSOCKOPT, }, + { + .descr = "setsockopt: ignore >PAGE_SIZE optlen", + .insns = { + /* write 0xFF to the first optval byte */ + + /* r6 = ctx->optval */ + BPF_LDX_MEM(BPF_DW, BPF_REG_6, BPF_REG_1, + offsetof(struct bpf_sockopt, optval)), + /* r2 = ctx->optval */ + BPF_MOV64_REG(BPF_REG_2, BPF_REG_6), + /* r6 = ctx->optval + 1 */ + BPF_ALU64_IMM(BPF_ADD, BPF_REG_6, 1), + + /* r7 = ctx->optval_end */ + BPF_LDX_MEM(BPF_DW, BPF_REG_7, BPF_REG_1, + offsetof(struct bpf_sockopt, optval_end)), + + /* if (ctx->optval + 1 <= ctx->optval_end) { */ + BPF_JMP_REG(BPF_JGT, BPF_REG_6, BPF_REG_7, 1), + /* ctx->optval[0] = 0xF0 */ + BPF_ST_MEM(BPF_B, BPF_REG_2, 0, 0xFF), + /* } */ + + BPF_MOV64_IMM(BPF_REG_0, 1), + BPF_EXIT_INSN(), + }, + .attach_type = BPF_CGROUP_SETSOCKOPT, + .expected_attach_type = BPF_CGROUP_SETSOCKOPT, + + .set_level = SOL_IP, + .set_optname = IP_TOS, + .set_optval = { 1 << 3 }, + .set_optlen = 4096 + 1, + + .get_level = SOL_IP, + .get_optname = IP_TOS, +#if __BYTE_ORDER == __LITTLE_ENDIAN + .get_optval = { 1 << 3, 0, 0, 0 }, /* the changes are ignored */ +#else + .get_optval = { 0, 0, 0, 1 << 3 }, /* the changes are ignored */ +#endif + .get_optlen = 4, + }, { .descr = "setsockopt: allow changing ctx->optlen within bounds", .insns = { @@ -922,6 +986,7 @@ static int run_test(int cgroup_fd, struct sockopt_test *test) if (test->get_optlen) { optval = malloc(test->get_optlen); + memset(optval, 0, test->get_optlen); socklen_t optlen = test->get_optlen; socklen_t expected_get_optlen = test->get_optlen_ret ?: test->get_optlen; @@ -946,6 +1011,9 @@ static int run_test(int cgroup_fd, struct sockopt_test *test) goto free_optval; } + if (optlen > sizeof(test->get_optval)) + optlen = sizeof(test->get_optval); + if (memcmp(optval, test->get_optval, optlen) != 0) { errno = 0; log_err("getsockopt returned unexpected optval");
Instead of assuming EFAULT, let's assume the BPF program's output is ignored. Remove "getsockopt: deny arbitrary ctx->retval" because it was actually testing optlen. We have separate set of tests for retval. Signed-off-by: Stanislav Fomichev <sdf@google.com> --- .../selftests/bpf/prog_tests/sockopt.c | 80 +++++++++++++++++-- 1 file changed, 74 insertions(+), 6 deletions(-)