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
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
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.
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.
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
linux-kselftest-mirror@lists.linaro.org
-
Dan Carpenter -
Kuniyuki Iwashima