Re: [syzbot] KMSAN: uninit-value in p9pdu_readf

[Dmitry Vyukov]

On Mon, 11 Oct 2021 at 08:54, <asmadeus@xxxxxxxxxxxxx> wrote:
>
> Thanks for the reply,
>
> Dmitry Vyukov wrote on Mon, Oct 11, 2021 at 07:56:05AM +0200:
> > > would be 'len' in p9pdu_vreadf, which has to be set as far as I can understand:
> > > > uint16_t len;
> > > >
> > > > errcode = p9pdu_readf(pdu, proto_version,
> > > >                                 "w", &len);
> > > > if (errcode)
> > > >         break;
> > > >
> > > > *sptr = kmalloc(len + 1, GFP_NOFS);
> > >
> > > with relevant part of p9pdu_readf being:
> > > > case 'w':{
> > > >                int16_t *val = va_arg(ap, int16_t *);
> > > >                __le16 le_val;
> > > >                if (pdu_read(pdu, &le_val, sizeof(le_val))) {
> > > >                        errcode = -EFAULT;
> > > >                        break;
> > > >                }
> > > >                *val = le16_to_cpu(le_val);
> > > >        }
> > > > ...
> > > > return errcode;
> > >
> > > e.g. either len or errcode should be set...
> > >
> > > But:
> > > > Local variable ----ecode@p9_check_errors created at:
> > > >  p9_check_errors+0x68/0xb90 net/9p/client.c:506
> > > >  p9_client_rpc+0xd90/0x1410 net/9p/client.c:801
> > >
> > > is something totally different, p9_client_rpc happens before the
> > > p9pdu_readf call in p9_client_stat, and ecode is local to
> > > p9_check_errors, I don't see how it could get that far.
> > >
> > > Note that inspecting p9_check_errors manually, there is a case where
> > > ecode is returned (indirectly through err = -ecode) without being
> > > initialized,
> >
> > Does this connect both stacks? So the uinit is ecode, is it used in
> > p9pdu_vreadf? If yes, then that's what KMSAN reported.
>
> Hm...
> Assuming that's the uninit, it'd have been propagated as the return
> value as req = p9_client_rpc; passed the IS_ERR(req) check as not an
> error, then passed to p9pdu_readf(&req->rc (later 'pdu')...)
> That would then try to read some undefined address in pdu_read() as
> memcpy(data, &pdu->sdata[pdu->offset], len)
> and returning another undefined value as
> sizeof(__le16) - min(pdu->size - pdu->offset, __le16)
>
> Here magic should happen that makes this neither a success (would set
> *val e.g. len in the kmalloc line that complains) or an error (would set
> errcode e.g. p9pdu_vreadf() would return before reaching that line)
>
> I guess with undefineds anything can happen and this is a valid link?
>
> I would have assumed kmsan checks would fail sooner but I don't see what
> else it could be.

KMSAN tracks propagation of uninits and reports only "uses".
Reporting sooner tends to produce lots of false positives because code
tends to operate/propagate/copy/add uninits, but then discard.


> > > so I will send a patch for that at least, but I have no
> > > idea if that is what has been reported and it should be trivial to
> > > reproduce so I do not see why syzbot does not have a reproducer -- it
> > > retries running the last program that triggered the error before sending
> > > the report, right?
> >
> > Yes.
>
> Ok, I guess there are conditions on the undefined value to reach this
> check down the road, even if the undefined return itself should be
> always reproducible.
>
> Either way Pavel Skripkin reached the same conclusion as me at roughly
> the same time so I'll just go with it.
>
> --
> Dominique