Re: [bug report] selftest: bpf: Test bpf_sk_assign_tcp_reqsk().

[Dan Carpenter <dan.carpenter@linaro.org>]

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