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

[asmadeus@codewreck.org]

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.


> > 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