[PATCH v2] net/9p: Initialize the iounit field during fid creation

[PATCH v2] net/9p: Initialize the iounit field during fid creation

[Tyler Hicks]

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@vger.kernel.org
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.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);
-- 
2.25.1

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

[Christian Schoenebeck]

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@vger.kernel.org
> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>

Reviewed-by: Christian Schoenebeck <linux_oss@crudebyte.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);

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

[Dominique Martinet]

Tyler Hicks wrote on Sun, Jul 10, 2022 at 09:14:02AM -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
> 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@vger.kernel.org
> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>

Thanks for the v2, looks good to me and tested quickly so I've queued it
up.
(and thanks all the fixes lately and for the reminder, too many patches
lately I thought I had already taken it... Feel free to send 'pings' on
the list)

Since the next merge window is close (probably starts next week-ish) I
won't bother with a separate PR for 5.19; it's been 12 years it can wait
another week.

--
Dominique