Re: [V9fs-developer] [PATCH] Integer underflow in pdu_read()

[Dominique Martinet <asmadeus@codewreck.org>]

Tomas Bortoli wrote on Tue, Jul 10, 2018:
> > What does cause the calls of pdu_read() in such conditions and shouldn't *that*
> > be dealt with?
>
> Mmh I think that's caused by p9_parse_header(). That function reads the
> first 7 bytes of a PDU regardless of the current offset. It then sets
> the PDU length to the one read and then it restores the original offset.
> Therefore, it's possible to set a size < offset here.
> (to be 100% sure I'd need more debugging)

It doesn't always restore the original offset; but if the packet lied
and said its size is < 7 it doesn't even need to.

I think as things stand it should be enough to fix it there, once the
state machine is runnning there don't seem to be any way of making
offset jump over size; but I'm not fussy either way, protecting in
pdu_read is probably just as good.
Note that there is another "min_t(uint32_t, *count, pdu->size -
pdu->offset)" that needs a similar check below in the file if you want
to go this way.

On the other hand, pdu_write() calls come from the system so hopefully
these are a bit more trustworthy, but I guess the extra check could
protect against a programming error.
I'm not sure what the linux kernel policy about these "redundant"
low-level checks is as this is technically in the fast path.

 
> We can prevent it in p9_parse_header(), but if the invariant offset <
> size gets broken elsewhere we fall back to the underflow. Enforcing it
> in pdu_read() should be the safest thing to do as it would detect *any*
> bad read.

The main problem is that 9p is just too trusty of what is sent to it.
To further extent what was said in the other thread ("[PATCH] KASAN:
slab-out-of-bounds Read in pdu_read") the extra check on pdu->size that
must be smaller than actual read size holds for p9_check_zc_error as
well so should probably be moved to p9_parse_header() ; but a packet
that says its size is < 7 is also wrong : for trans_fd, we are guaranted
to have read as much by the transport, so once again size didn't match
up.
Ideally, pdu->size should only ever be set by the transport who know
what they have read, and p9_parse_header should check that r_size ==
pdu->size and error if not.


Also, as TCP is a stream protocol, once we've had a single packet lie
about its size we're lost in the stream with no chance of recovering so
the connection should probably be aborted.
For rdma/virtio there is a notion of packet so they could recover, but
TCP is not as forgiving...

-- 
Dominique Martinet | Asmadeus