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] [day] [month] [year] [list]

Message-ID: <3743363.vbEDF3eA3V@silver>
Date:   Fri, 18 Nov 2022 16:34:35 +0100
From:   Christian Schoenebeck <linux_oss@...debyte.com>
To:     "Guozihua (Scott)" <guozihua@...wei.com>, asmadeus@...ewreck.org
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 Friday, November 18, 2022 2:57:14 PM CET asmadeus@...ewreck.org wrote:
> Guozihua (Scott) wrote on Fri, Nov 18, 2022 at 06:18:16PM +0800:
> > I retried the repro on your branch, the issue does not reproduce. What
> > a good pair of eyes :)！
> 
> Thanks!
> By the way the original check also compared size to msize directly,
> without an offset for headers, so with hindsight it looks clear enough
> that the size is the full size including the header.
> 
> I'm not sure why I convinced myself it didn't...
> 
> Anyway, this made me check other places where we might fail at this and
> I've a couple more patches; please review if you have time.
> I'll send them all to Linus next week...
> 

Aah, the offset is already incremented before that block is entered:

303	err = p9_fd_read(m->client, m->rc.sdata + m->rc.offset,
304			 m->rc.capacity - m->rc.offset);
...
312	m->rc.offset += err;
313
314	/* header read in */
315	if ((!m->rreq) && (m->rc.offset == m->rc.capacity)) {

And the data is then copied to m->rreq->rc.sdata without any offset. So yes,
there should be no `offset` in the check.

Best regards,
Christian Schoenebeck