netdev - Re: [PATCH] net/9p: Initialize the iounit field during fid creation

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]

Date:   Sun, 10 Jul 2022 09:12:51 -0500
From:   Tyler Hicks <tyhicks@...ux.microsoft.com>
To:     Dominique Martinet <asmadeus@...ewreck.org>,
        Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc:     Eric Van Hensbergen <ericvh@...il.com>,
        Latchesar Ionkov <lucho@...kov.net>,
        Christian Schoenebeck <linux_oss@...debyte.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        v9fs-developer@...ts.sourceforge.net, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net/9p: Initialize the iounit field during fid creation

On 2022-07-10 22:21:33, Dominique Martinet wrote:
> Tyler Hicks wrote on Sat, Jul 09, 2022 at 03:00:05PM -0500:
> > Ensure that the fid's iounit field is set to zero when a new fid is
> > created. Certain 9P operations, such as OPEN and CREATE, allow the
> > server to reply with an iounit size which the client code assigns to the
> > fid struct shortly after the fid is created in p9_fid_create(). Other
> > operations that follow a call to p9_fid_create(), such as an XATTRWALK,
> > don't include an iounit value in the reply message from the server. In
> > the latter case, the iounit field remained uninitialized. Depending on
> > allocation patterns, the iounit value could have been something
> > reasonable that was carried over from previously freed fids or, in the
> > worst case, could have been arbitrary values from non-fid related usages
> > of the memory location.
> > 
> > The bug was detected in the Windows Subsystem for Linux 2 (WSL2) kernel
> > after the uninitialized iounit field resulted in the typical sequence of
> > two getxattr(2) syscalls, one to get the size of an xattr and another
> > after allocating a sufficiently sized buffer to fit the xattr value, to
> > hit an unexpected ERANGE error in the second call to getxattr(2). An
> > uninitialized iounit field would sometimes force rsize to be smaller
> > than the xattr value size in p9_client_read_once() and the 9P server in
> > WSL refused to chunk up the READ on the attr_fid and, instead, returned
> > ERANGE to the client. The virtfs server in QEMU seems happy to chunk up
> > the READ and this problem goes undetected there. However, there are
> > likely other non-xattr implications of this bug that could cause
> > inefficient communication between the client and server.
> > 
> > Cc: stable@...r.kernel.org
> > Signed-off-by: Tyler Hicks <tyhicks@...ux.microsoft.com>
> 
> Thanks for the fix!

No problem!

> 
> > ---
> > 
> > Note that I haven't had a chance to identify when this bug was
> > introduced so I don't yet have a proper Fixes tag. The history looked a
> > little tricky to me but I'll have another look in the coming days. We
> > started hitting this bug after trying to move from linux-5.10.y to
> > linux-5.15.y but I didn't see any obvious changes between those two
> > series. I'm not confident of this theory but perhaps the fid refcounting
> > changes impacted the fid allocation patterns enough to uncover the
> > latent bug?
> > 
> >  net/9p/client.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/net/9p/client.c b/net/9p/client.c
> > index 8bba0d9cf975..1dfceb9154f7 100644
> > --- a/net/9p/client.c
> > +++ b/net/9p/client.c
> > @@ -899,6 +899,7 @@ static struct p9_fid *p9_fid_create(struct p9_client *clnt)
> >  	fid->clnt = clnt;
> >  	fid->rdir = NULL;
> >  	fid->fid = 0;
> > +	fid->iounit = 0;
> 
> ugh, this isn't the first we've missed so I'll be tempted to agree with
> Christophe -- let's make that a kzalloc and only set non-zero fields.

Agreed - This is the better approach. V2 will be sent out shortly.

Tyler

> 
> --
> Dominique
>