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]

Date:   Fri, 18 Nov 2022 13:59:18 +0900
From:   asmadeus@...ewreck.org
To:     Christian Schoenebeck <linux_oss@...debyte.com>
Cc:     ericvh@...il.com, lucho@...kov.net,
        GUO Zihua <guozihua@...wei.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 0/3 v2] 9p: Fix write overflow in p9_read_work

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.

-- 
Dominique