netdev - Re: [PATCH 0/3 v2] 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <a6aec93a-1166-1d8a-48de-767bc1eb2214@huawei.com>
Date:   Fri, 18 Nov 2022 18:18:16 +0800
From:   "Guozihua (Scott)" <guozihua@...wei.com>
To:     <asmadeus@...ewreck.org>,
        Christian Schoenebeck <linux_oss@...debyte.com>
CC:     <ericvh@...il.com>, <lucho@...kov.net>, <davem@...emloft.net>,
        <edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>,
        <v9fs-developer@...ts.sourceforge.net>, <netdev@...r.kernel.org>
Subject: Re: [PATCH 0/3 v2] 9p: Fix write overflow in p9_read_work

On 2022/11/18 12:59, asmadeus@...ewreck.org wrote:
> Christian Schoenebeck wrote on Thu, Nov 17, 2022 at 02:33:28PM +0100:
>>> GUO Zihua (3):
>>>    9p: Fix write overflow in p9_read_work
>>>    9p: Remove redundent checks for message size against msize.
>>>    9p: Use P9_HDRSZ for header size
>>
>> For entire series:
>>
>> Reviewed-by: Christian Schoenebeck <linux_oss@...debyte.com>
>>
>> I agree with Dominique that patch 1 and 2 should be merged.
> 
> Thank you both!
> 
> I've just pushed the patches to my next branch:
> https://github.com/martinetd/linux/commits/9p-next
> 
> ... with a twist, as the original patch fails on any normal workload:
> ---
> / # ls /mnt
> 9pnet: -- p9_read_work (19): requested packet size too big: 9 for tag 0 with capacity 11
> ---
> (so much for having two pairs of eyes :-D
> By the way we -really- need to replace P9_DEBUG_ERROR by pr_error or
> something, these should be displayed without having to specify
> debug=1...)
> 
> 
> capacity is only set in a handful of places (alloc time, hardcoded 7 in
> trans_fd, after receiving packet) so I've added logs and our alloc
> really passed '11' for alloc_rsize (logging tsize/rsize)
> 
> 9pnet: (00000087) >>> TWALK fids 1,2 nwname 0d wname[0] (null)
> 9pnet: -- p9_tag_alloc (87): allocating capacity to 17/11 for tag 0
> 9pnet: -- p9_read_work (19): requested packet size too big: 9 for tag 0 with capacity 11
> 
> ... So this is RWALK, right:
> size[4] Rwalk tag[2] nwqid[2] nwqid*(wqid[13])
> 4 ..... 5.... 7..... 9....... packet end at 9 as 0 nwqid.
> We have capacity 11 to allow rlerror_size which is bigger; everything is
> good.
> 
> Long story short, the size header includes the header size, when I
> misread https://9fans.github.io/plan9port/man/man9/version.html to
> say it didn't (it just says it doesn't include the enveloping transport
> protocol, it starts from size alright and I just misread that)
> Thanksfully the code caught it.
> 
> So I've just removed the - offset part and things appear to work again.
> 
> Guo Zihua, can you check this still fixes your syz repro, or was that
> substraction needed? If it's still needed we have an off by 1 somewhere
> to look for.
> 

Hi Dominique, I retried the repro on your branch, the issue does not 
reproduce. What a good pair of eyes :)！

-- 
Best
GUO Zihua