Re: [PATCH] 9p: Fix write overflow in p9_read_work

[asmadeus@codewreck.org]

GUO Zihua wrote on Thu, Nov 17, 2022 at 02:14:44PM +0800:
> The root cause of this issue is that we check the size of the message
> received against the msize of the client in p9_read_work. However, this
> msize could be lager than the capacity of the sdata buffer. Thus,
> the message size should also be checked against sdata capacity.

Thanks for the fix!

I'm picky, so a few remarks below.

> 
> Reported-by: syzbot+0f89bd13eaceccc0e126@syzkaller.appspotmail.com
> Fixes: 1b0a763bdd5e ("9p: use the rcall structure passed in the request in trans_fd read_work")
> Signed-off-by: GUO Zihua <guozihua@huawei.com>
> ---
>  net/9p/trans_fd.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
> index 56a186768750..bc131a21c098 100644
> --- a/net/9p/trans_fd.c
> +++ b/net/9p/trans_fd.c
> @@ -342,6 +342,14 @@ static void p9_read_work(struct work_struct *work)
>  			goto error;
>  		}
>  
> +		if (m->rc.size > m->rreq->rc.capacity - m->rc.offset) {

Ah, it took me a while to understand but capacity here is no longer the
same as msize since commit 60ece0833b6c ("net/9p: allocate appropriate
reduced message buffers")

If you have time to test the reproducer, please check with any commit
before 60ece0833b6c if you can still reproduce. If not please fix your
Fixes tag to this commit.
I'd appreciate a word in the commit message saying that message capacity
is no longer constant here and needs a more subtle check than msize.


Also:
 - We can remove the msize check, it's redundant with this; it doesn't
matter if we don't check for msize before doing the tag lookup as tag
has already been read
 - While the `- offset` part of the check is correct (rc.size does
not include headers, and the current offset must be 7 here) I'd prefer
if you woud use P9_HDRSZ as that's defined in the protocol and using
macros will be easier to check if that ever evolves.
 - (I'd also appreciate if you could update the capacity = 7 next to the
'start by reading header' comment above while you're here so we use the
same macro in both place)


> +			p9_debug(P9_DEBUG_ERROR,
> +				 "requested packet size too big: %d\n",
> +				 m->rc.size);

Please log m->rc.tag, m->rc.id and m->rreq->rc.capacity as well for
debugging if that happens.

> +			err = -EIO;
> +			goto error;
> +		}
> +
>  		if (!m->rreq->rc.sdata) {
>  			p9_debug(P9_DEBUG_ERROR,
> 				 "No recv fcall for tag %d (req %p), disconnecting!\n",
--
Dominique