From: Martin KaFai Lau <martin.lau@linux.dev>
To: Stanislav Fomichev <sdf@google.com>
Cc: ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org,
song@kernel.org, yhs@fb.com, john.fastabend@gmail.com,
kpsingh@kernel.org, haoluo@google.com, jolsa@kernel.org,
bpf@vger.kernel.org
Subject: Re: [PATCH bpf-next v3 2/4] selftests/bpf: Update EFAULT {g,s}etsockopt selftests
Date: Tue, 2 May 2023 17:42:56 -0700 [thread overview]
Message-ID: <00537b92-c8f8-e101-1016-0ee980b028f1@linux.dev> (raw)
In-Reply-To: <9cc9a5f6-35cd-cfa3-8034-18dac9f20d6f@linux.dev>
On 5/2/23 5:29 PM, Martin KaFai Lau wrote:
> On 5/1/23 12:48 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 | 98 +++++++++++++++++--
>> 1 file changed, 92 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..a7bc9dc93ce0 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/sockopt.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/sockopt.c
>> @@ -5,6 +5,10 @@
>> static char bpf_log_buf[4096];
>> static bool verbose;
>> +#ifndef PAGE_SIZE
>> +#define PAGE_SIZE 4096
>> +#endif
>> +
>> enum sockopt_test_error {
>> OK = 0,
>> DENY_LOAD,
>> @@ -273,10 +277,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),
>
>
> This is an interesting test case. One more question just came to my mind,
> does it make sense to also ignore the bpf-prog's 'ctx->retval = 0' in getsockopt
> considering its optval change has already been ignored. Something like:
>
> if (optval && (ctx.optlen > max_optlen || ctx.optlen < 0)) {
> if (orig_optlen > PAGE_SIZE && ctx.optlen >= 0) {
> pr_info_once("bpf getsockopt: ignoring program buffer with
> optlen=%d (max_optlen=%d)\n",
> ctx.optlen, max_optlen);
> ret = retval;
> goto out;
> }
> ret = -EFAULT;
> goto out;
> }
Previous one has indentation off. Meaning to be:
if (optval && (ctx.optlen > max_optlen || ctx.optlen < 0)) {
if (orig_optlen > PAGE_SIZE && ctx.optlen >= 0) {
pr_info_once("bpf getsockopt: ignoring program buffer with optlen=%d
(max_optlen=%d)\n",
ctx.optlen, max_optlen);
ret = retval;
goto out;
}
ret = -EFAULT;
goto out;
}
>
>
>> BPF_STX_MEM(BPF_W, BPF_REG_1, BPF_REG_0,
>> offsetof(struct bpf_sockopt, retval)),
>> @@ -287,9 +311,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 = PAGE_SIZE + 1,
>> }
>> + 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");
>
next prev parent reply other threads:[~2023-05-03 0:43 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-01 19:48 [PATCH bpf-next v3 0/4] bpf: Don't EFAULT for {g,s}setsockopt with wrong optlen Stanislav Fomichev
2023-05-01 19:48 ` [PATCH bpf-next v3 1/4] " Stanislav Fomichev
2023-05-01 19:48 ` [PATCH bpf-next v3 2/4] selftests/bpf: Update EFAULT {g,s}etsockopt selftests Stanislav Fomichev
2023-05-03 0:29 ` Martin KaFai Lau
2023-05-03 0:42 ` Martin KaFai Lau [this message]
2023-05-03 18:27 ` Stanislav Fomichev
2023-05-01 19:48 ` [PATCH bpf-next v3 3/4] selftests/bpf: Correctly handle optlen > 4096 Stanislav Fomichev
2023-05-01 19:48 ` [PATCH bpf-next v3 4/4] bpf: Document EFAULT changes for sockopt Stanislav Fomichev
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=00537b92-c8f8-e101-1016-0ee980b028f1@linux.dev \
--to=martin.lau@linux.dev \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=haoluo@google.com \
--cc=john.fastabend@gmail.com \
--cc=jolsa@kernel.org \
--cc=kpsingh@kernel.org \
--cc=sdf@google.com \
--cc=song@kernel.org \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.