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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox