linux-kernel - Re: [PATCH v2] 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <1984068.YOKu8ataPd@silver>
Date:   Sun, 10 Jul 2022 16:48:29 +0200
From:   Christian Schoenebeck <linux_oss@...debyte.com>
To:     Dominique Martinet <asmadeus@...ewreck.org>,
        Tyler Hicks <tyhicks@...ux.microsoft.com>
Cc:     Eric Van Hensbergen <ericvh@...il.com>,
        Latchesar Ionkov <lucho@...kov.net>,
        Christophe JAILLET <christophe.jaillet@...adoo.fr>,
        "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 v2] net/9p: Initialize the iounit field during fid creation

On Sonntag, 10. Juli 2022 16:14:02 CEST Tyler Hicks wrote:
> 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
> p9_fid struct shortly after the fid is created by p9_fid_create(). On
> the other hand, an XATTRWALK operation doesn't allow for the server to
> specify an iounit value. The iounit field of the newly allocated p9_fid
> struct remained uninitialized in that case. 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.
> 
> Fixes: ebf46264a004 ("fs/9p: Add support user. xattr")
> Cc: stable@...r.kernel.org
> Signed-off-by: Tyler Hicks <tyhicks@...ux.microsoft.com>

Reviewed-by: Christian Schoenebeck <linux_oss@...debyte.com>

> ---
> 
> v2:
> - Add Fixes tag
> - Improve commit message clarity to make it clear that this only affects
>   xattr get/set
> - kzalloc() the entire fid struct instead of individually zeroing each
>   member
>   - Thanks to Christophe JAILLET for the suggestion
> v1: https://lore.kernel.org/lkml/20220710062557.GA272934@sequoia/
> 
>  net/9p/client.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/net/9p/client.c b/net/9p/client.c
> index 8bba0d9cf975..371519e7b885 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -889,16 +889,13 @@ static struct p9_fid *p9_fid_create(struct p9_client
> *clnt) struct p9_fid *fid;
> 
>  	p9_debug(P9_DEBUG_FID, "clnt %p\n", clnt);
> -	fid = kmalloc(sizeof(*fid), GFP_KERNEL);
> +	fid = kzalloc(sizeof(*fid), GFP_KERNEL);
>  	if (!fid)
>  		return NULL;
> 
> -	memset(&fid->qid, 0, sizeof(fid->qid));
>  	fid->mode = -1;
>  	fid->uid = current_fsuid();
>  	fid->clnt = clnt;
> -	fid->rdir = NULL;
> -	fid->fid = 0;
>  	refcount_set(&fid->count, 1);
> 
>  	idr_preload(GFP_KERNEL);