On Mon, Aug 19, 2024 at 12:14:13PM -0700, Kuniyuki Iwashima wrote:
> From: Kuniyuki Iwashima <kuniyu@xxxxxxxxxx>
> 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
