netdev - Re: [PATCH] 9p: Fix write overflow in p9_read

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1fe5dc97-f6f6-f204-bbf5-954fe7e9790c@huawei.com>
Date:   Thu, 17 Nov 2022 15:58:09 +0800
From:   "Guozihua (Scott)" <guozihua@...wei.com>
To:     <asmadeus@...ewreck.org>
CC:     <ericvh@...il.com>, <lucho@...kov.net>, <linux_oss@...debyte.com>,
        <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
        <pabeni@...hat.com>, <v9fs-developer@...ts.sourceforge.net>,
        <netdev@...r.kernel.org>
Subject: Re: [PATCH] 9p: Fix write overflow in p9_read_work

On 2022/11/17 15:29, asmadeus@...ewreck.org wrote:
> 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@...kaller.appspotmail.com
>> Fixes: 1b0a763bdd5e ("9p: use the rcall structure passed in the request in trans_fd read_work")
>> Signed-off-by: GUO Zihua <guozihua@...wei.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

Hi Dominique, Thanks for the comment, will push a v2 right away.
-- 
Best
GUO Zihua