[PATCH] nfsd: encoders mustn't use unitialized values in error cases

[PATCH] nfsd: encoders mustn't use unitialized values in error cases

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

In error cases, lgp->lg_layout_type may be out of bounds; so we
shouldn't be using it until after the check of nfserr.

This was seen to crash nfsd threads when the server receives a LAYOUTGET
request with a large layout type.

GETDEVICEINFO has the same problem.

Reported-by: Ari Kauppi <Ari.Kauppi@synopsys.com>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/nfs4xdr.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 33017d652b1d..8e9652f37f1f 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -4119,8 +4119,7 @@ nfsd4_encode_getdeviceinfo(struct nfsd4_compoundres *resp, __be32 nfserr,
 		struct nfsd4_getdeviceinfo *gdev)
 {
 	struct xdr_stream *xdr = &resp->xdr;
-	const struct nfsd4_layout_ops *ops =
-		nfsd4_layout_ops[gdev->gd_layout_type];
+	const struct nfsd4_layout_ops *ops;
 	u32 starting_len = xdr->buf->len, needed_len;
 	__be32 *p;
 
@@ -4137,6 +4136,7 @@ nfsd4_encode_getdeviceinfo(struct nfsd4_compoundres *resp, __be32 nfserr,
 
 	/* If maxcount is 0 then just update notifications */
 	if (gdev->gd_maxcount != 0) {
+		ops = nfsd4_layout_ops[gdev->gd_layout_type];
 		nfserr = ops->encode_getdeviceinfo(xdr, gdev);
 		if (nfserr) {
 			/*
@@ -4189,8 +4189,7 @@ nfsd4_encode_layoutget(struct nfsd4_compoundres *resp, __be32 nfserr,
 		struct nfsd4_layoutget *lgp)
 {
 	struct xdr_stream *xdr = &resp->xdr;
-	const struct nfsd4_layout_ops *ops =
-		nfsd4_layout_ops[lgp->lg_layout_type];
+	const struct nfsd4_layout_ops *ops;
 	__be32 *p;
 
 	dprintk("%s: err %d\n", __func__, nfserr);
@@ -4213,6 +4212,7 @@ nfsd4_encode_layoutget(struct nfsd4_compoundres *resp, __be32 nfserr,
 	*p++ = cpu_to_be32(lgp->lg_seg.iomode);
 	*p++ = cpu_to_be32(lgp->lg_layout_type);
 
+	ops = nfsd4_layout_ops[lgp->lg_layout_type];
 	nfserr = ops->encode_layoutget(xdr, lgp);
 out:
 	kfree(lgp->lg_content);
-- 
2.9.3

Re: [PATCH] nfsd: encoders mustn't use unitialized values in error cases

[J. Bruce Fields]

I'm also thinking of patching the server to bypass most of the
individual encoders in the error case.  There's only a couple that
actually do anything there, most of just start with variations on

	if (nfserr)
		return nfserr;

so I figure it's better to catch that once rather than giving them a
chance to mess it up.

I also wondered for a moment if there's a module-unload race here....
But the layout types aren't loadable modules on the server side, so OK.

--b.

On Mon, May 08, 2017 at 11:57:41AM -0400, bfields wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> In error cases, lgp->lg_layout_type may be out of bounds; so we
> shouldn't be using it until after the check of nfserr.
> 
> This was seen to crash nfsd threads when the server receives a LAYOUTGET
> request with a large layout type.
> 
> GETDEVICEINFO has the same problem.
> 
> Reported-by: Ari Kauppi <Ari.Kauppi@synopsys.com>
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
>  fs/nfsd/nfs4xdr.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 33017d652b1d..8e9652f37f1f 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -4119,8 +4119,7 @@ nfsd4_encode_getdeviceinfo(struct nfsd4_compoundres *resp, __be32 nfserr,
>  		struct nfsd4_getdeviceinfo *gdev)
>  {
>  	struct xdr_stream *xdr = &resp->xdr;
> -	const struct nfsd4_layout_ops *ops =
> -		nfsd4_layout_ops[gdev->gd_layout_type];
> +	const struct nfsd4_layout_ops *ops;
>  	u32 starting_len = xdr->buf->len, needed_len;
>  	__be32 *p;
>  
> @@ -4137,6 +4136,7 @@ nfsd4_encode_getdeviceinfo(struct nfsd4_compoundres *resp, __be32 nfserr,
>  
>  	/* If maxcount is 0 then just update notifications */
>  	if (gdev->gd_maxcount != 0) {
> +		ops = nfsd4_layout_ops[gdev->gd_layout_type];
>  		nfserr = ops->encode_getdeviceinfo(xdr, gdev);
>  		if (nfserr) {
>  			/*
> @@ -4189,8 +4189,7 @@ nfsd4_encode_layoutget(struct nfsd4_compoundres *resp, __be32 nfserr,
>  		struct nfsd4_layoutget *lgp)
>  {
>  	struct xdr_stream *xdr = &resp->xdr;
> -	const struct nfsd4_layout_ops *ops =
> -		nfsd4_layout_ops[lgp->lg_layout_type];
> +	const struct nfsd4_layout_ops *ops;
>  	__be32 *p;
>  
>  	dprintk("%s: err %d\n", __func__, nfserr);
> @@ -4213,6 +4212,7 @@ nfsd4_encode_layoutget(struct nfsd4_compoundres *resp, __be32 nfserr,
>  	*p++ = cpu_to_be32(lgp->lg_seg.iomode);
>  	*p++ = cpu_to_be32(lgp->lg_layout_type);
>  
> +	ops = nfsd4_layout_ops[lgp->lg_layout_type];
>  	nfserr = ops->encode_layoutget(xdr, lgp);
>  out:
>  	kfree(lgp->lg_content);
> -- 
> 2.9.3
> 

Re: [PATCH] nfsd: encoders mustn't use unitialized values in error cases

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>