* [bug report] selftest: bpf: Test bpf_sk_assign_tcp_reqsk().
@ 2024-08-19 18:54 Dan Carpenter
2024-08-19 18:57 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2024-08-19 18:54 UTC (permalink / raw)
To: Kuniyuki Iwashima; +Cc: bpf, linux-kselftest
Hello Kuniyuki Iwashima,
Commit a74712241b46 ("selftest: bpf: Test
bpf_sk_assign_tcp_reqsk().") from Jan 15, 2024 (linux-next), leads to
the following Smatch static checker warning:
tools/testing/selftests/bpf/progs/test_tcp_custom_syncookie.c:493 tcp_validate_cookie()
warn: off by one 'mssind' == ARRAY_SIZE()?
./tools/testing/selftests/bpf/progs/test_tcp_custom_syncookie.c
462 static int tcp_validate_cookie(struct tcp_syncookie *ctx)
463 {
464 u32 cookie = bpf_ntohl(ctx->tcp->ack_seq) - 1;
465 u32 seq = bpf_ntohl(ctx->tcp->seq) - 1;
466 u64 first = 0, second;
467 int mssind;
468 u32 hash;
469
470 if (ctx->ipv4)
471 first = (u64)ctx->ipv4->saddr << 32 | ctx->ipv4->daddr;
472 else if (ctx->ipv6)
473 first = (u64)ctx->ipv6->saddr.in6_u.u6_addr8[0] << 32 |
474 ctx->ipv6->daddr.in6_u.u6_addr32[0];
475
476 second = (u64)seq << 32 | ctx->tcp->source << 16 | ctx->tcp->dest;
477 hash = siphash_2u64(first, second, &test_key_siphash);
478
479 if (ctx->attrs.tstamp_ok)
480 hash -= ctx->attrs.rcv_tsecr & COOKIE_MASK;
481 else
482 hash &= ~COOKIE_MASK;
483
484 hash -= cookie & ~COOKIE_MASK;
485 if (hash)
486 goto err;
487
488 mssind = (cookie & (3 << 6)) >> 6;
489 if (ctx->ipv4) {
490 if (mssind > ARRAY_SIZE(msstab4))
^
Should be >= instead of >.
491 goto err;
492
--> 493 ctx->attrs.mss = msstab4[mssind];
494 } else {
495 if (mssind > ARRAY_SIZE(msstab6))
496 goto err;
497
498 ctx->attrs.mss = msstab6[mssind];
499 }
500
501 ctx->attrs.snd_wscale = cookie & BPF_SYNCOOKIE_WSCALE_MASK;
502 ctx->attrs.rcv_wscale = ctx->attrs.snd_wscale;
503 ctx->attrs.wscale_ok = ctx->attrs.snd_wscale == BPF_SYNCOOKIE_WSCALE_MASK;
504 ctx->attrs.sack_ok = cookie & BPF_SYNCOOKIE_SACK;
505 ctx->attrs.ecn_ok = cookie & BPF_SYNCOOKIE_ECN;
506
507 return 0;
508 err:
509 return -1;
510 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] selftest: bpf: Test bpf_sk_assign_tcp_reqsk().
2024-08-19 18:54 [bug report] selftest: bpf: Test bpf_sk_assign_tcp_reqsk() Dan Carpenter
@ 2024-08-19 18:57 ` Dan Carpenter
2024-08-19 19:07 ` Kuniyuki Iwashima
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2024-08-19 18:57 UTC (permalink / raw)
To: Kuniyuki Iwashima; +Cc: bpf, linux-kselftest
On Mon, Aug 19, 2024 at 09:54:00PM +0300, Dan Carpenter wrote:
> Hello Kuniyuki Iwashima,
>
> Commit a74712241b46 ("selftest: bpf: Test
> bpf_sk_assign_tcp_reqsk().") from Jan 15, 2024 (linux-next), leads to
> the following Smatch static checker warning:
>
> tools/testing/selftests/bpf/progs/test_tcp_custom_syncookie.c:493 tcp_validate_cookie()
> warn: off by one 'mssind' == ARRAY_SIZE()?
>
> ./tools/testing/selftests/bpf/progs/test_tcp_custom_syncookie.c
> 462 static int tcp_validate_cookie(struct tcp_syncookie *ctx)
> 463 {
> 464 u32 cookie = bpf_ntohl(ctx->tcp->ack_seq) - 1;
> 465 u32 seq = bpf_ntohl(ctx->tcp->seq) - 1;
> 466 u64 first = 0, second;
> 467 int mssind;
> 468 u32 hash;
> 469
> 470 if (ctx->ipv4)
> 471 first = (u64)ctx->ipv4->saddr << 32 | ctx->ipv4->daddr;
> 472 else if (ctx->ipv6)
> 473 first = (u64)ctx->ipv6->saddr.in6_u.u6_addr8[0] << 32 |
> 474 ctx->ipv6->daddr.in6_u.u6_addr32[0];
> 475
> 476 second = (u64)seq << 32 | ctx->tcp->source << 16 | ctx->tcp->dest;
> 477 hash = siphash_2u64(first, second, &test_key_siphash);
> 478
> 479 if (ctx->attrs.tstamp_ok)
> 480 hash -= ctx->attrs.rcv_tsecr & COOKIE_MASK;
> 481 else
> 482 hash &= ~COOKIE_MASK;
> 483
> 484 hash -= cookie & ~COOKIE_MASK;
> 485 if (hash)
> 486 goto err;
> 487
> 488 mssind = (cookie & (3 << 6)) >> 6;
> 489 if (ctx->ipv4) {
> 490 if (mssind > ARRAY_SIZE(msstab4))
> ^
> Should be >= instead of >.
>
> 491 goto err;
> 492
> --> 493 ctx->attrs.mss = msstab4[mssind];
> 494 } else {
> 495 if (mssind > ARRAY_SIZE(msstab6))
^
Here too, I guess.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] selftest: bpf: Test bpf_sk_assign_tcp_reqsk().
2024-08-19 18:57 ` Dan Carpenter
@ 2024-08-19 19:07 ` Kuniyuki Iwashima
2024-08-19 19:14 ` Kuniyuki Iwashima
0 siblings, 1 reply; 5+ messages in thread
From: Kuniyuki Iwashima @ 2024-08-19 19:07 UTC (permalink / raw)
To: dan.carpenter; +Cc: bpf, kuniyu, linux-kselftest
From: Dan Carpenter <dan.carpenter@linaro.org>
Date: Mon, 19 Aug 2024 21:57:57 +0300
> On Mon, Aug 19, 2024 at 09:54:00PM +0300, Dan Carpenter wrote:
> > Hello Kuniyuki Iwashima,
> >
> > Commit a74712241b46 ("selftest: bpf: Test
> > bpf_sk_assign_tcp_reqsk().") from Jan 15, 2024 (linux-next), leads to
> > the following Smatch static checker warning:
> >
> > tools/testing/selftests/bpf/progs/test_tcp_custom_syncookie.c:493 tcp_validate_cookie()
> > warn: off by one 'mssind' == ARRAY_SIZE()?
> >
> > ./tools/testing/selftests/bpf/progs/test_tcp_custom_syncookie.c
> > 462 static int tcp_validate_cookie(struct tcp_syncookie *ctx)
> > 463 {
> > 464 u32 cookie = bpf_ntohl(ctx->tcp->ack_seq) - 1;
> > 465 u32 seq = bpf_ntohl(ctx->tcp->seq) - 1;
> > 466 u64 first = 0, second;
> > 467 int mssind;
> > 468 u32 hash;
> > 469
> > 470 if (ctx->ipv4)
> > 471 first = (u64)ctx->ipv4->saddr << 32 | ctx->ipv4->daddr;
> > 472 else if (ctx->ipv6)
> > 473 first = (u64)ctx->ipv6->saddr.in6_u.u6_addr8[0] << 32 |
> > 474 ctx->ipv6->daddr.in6_u.u6_addr32[0];
> > 475
> > 476 second = (u64)seq << 32 | ctx->tcp->source << 16 | ctx->tcp->dest;
> > 477 hash = siphash_2u64(first, second, &test_key_siphash);
> > 478
> > 479 if (ctx->attrs.tstamp_ok)
> > 480 hash -= ctx->attrs.rcv_tsecr & COOKIE_MASK;
> > 481 else
> > 482 hash &= ~COOKIE_MASK;
> > 483
> > 484 hash -= cookie & ~COOKIE_MASK;
> > 485 if (hash)
> > 486 goto err;
> > 487
> > 488 mssind = (cookie & (3 << 6)) >> 6;
> > 489 if (ctx->ipv4) {
> > 490 if (mssind > ARRAY_SIZE(msstab4))
> > ^
> > Should be >= instead of >.
> >
> > 491 goto err;
> > 492
> > --> 493 ctx->attrs.mss = msstab4[mssind];
> > 494 } else {
> > 495 if (mssind > ARRAY_SIZE(msstab6))
> ^
>
> Here too, I guess.
Thanks for reporting.
Will fix it.
But I'm curious why BPF verifier couldn't catch it.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] selftest: bpf: Test bpf_sk_assign_tcp_reqsk().
2024-08-19 19:07 ` Kuniyuki Iwashima
@ 2024-08-19 19:14 ` Kuniyuki Iwashima
2024-08-19 19:32 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Kuniyuki Iwashima @ 2024-08-19 19:14 UTC (permalink / raw)
To: kuniyu; +Cc: bpf, dan.carpenter, linux-kselftest
From: Kuniyuki Iwashima <kuniyu@amazon.com>
Date: Mon, 19 Aug 2024 12:07:04 -0700
> > > 488 mssind = (cookie & (3 << 6)) >> 6;
> > > 489 if (ctx->ipv4) {
> > > 490 if (mssind > ARRAY_SIZE(msstab4))
> > > ^
> > > Should be >= instead of >.
> > >
> > > 491 goto err;
> > > 492
> > > --> 493 ctx->attrs.mss = msstab4[mssind];
> > > 494 } else {
> > > 495 if (mssind > ARRAY_SIZE(msstab6))
> > ^
> >
> > Here too, I guess.
>
> Thanks for reporting.
>
> Will fix it.
>
> But I'm curious why BPF verifier couldn't catch it.
Ok, this off-by-one report is false-positive as the test has
mssind = (cookie & (3 << 6)) >> 6;
and the following (mssind > ARRAY_SIZE()) is just to make verifier happy.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] selftest: bpf: Test bpf_sk_assign_tcp_reqsk().
2024-08-19 19:14 ` Kuniyuki Iwashima
@ 2024-08-19 19:32 ` Dan Carpenter
0 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2024-08-19 19:32 UTC (permalink / raw)
To: Kuniyuki Iwashima; +Cc: bpf, linux-kselftest
On Mon, Aug 19, 2024 at 12:14:13PM -0700, Kuniyuki Iwashima wrote:
> From: Kuniyuki Iwashima <kuniyu@amazon.com>
> Date: Mon, 19 Aug 2024 12:07:04 -0700
> > > > 488 mssind = (cookie & (3 << 6)) >> 6;
> > > > 489 if (ctx->ipv4) {
> > > > 490 if (mssind > ARRAY_SIZE(msstab4))
> > > > ^
> > > > Should be >= instead of >.
> > > >
> > > > 491 goto err;
> > > > 492
> > > > --> 493 ctx->attrs.mss = msstab4[mssind];
> > > > 494 } else {
> > > > 495 if (mssind > ARRAY_SIZE(msstab6))
> > > ^
> > >
> > > Here too, I guess.
> >
> > Thanks for reporting.
> >
> > Will fix it.
> >
> > But I'm curious why BPF verifier couldn't catch it.
>
> Ok, this off-by-one report is false-positive as the test has
>
> mssind = (cookie & (3 << 6)) >> 6;
>
> and the following (mssind > ARRAY_SIZE()) is just to make verifier happy.
In this case, I was testing code that Smatch couldn't parse completely.
But also I have a different check for "> ARRAY_SIZE()" which deliberately
ignores the value of mssind since I was missing "false positive" bugs like this.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-08-19 19:32 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-19 18:54 [bug report] selftest: bpf: Test bpf_sk_assign_tcp_reqsk() Dan Carpenter
2024-08-19 18:57 ` Dan Carpenter
2024-08-19 19:07 ` Kuniyuki Iwashima
2024-08-19 19:14 ` Kuniyuki Iwashima
2024-08-19 19:32 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox