[PATCH v2 0/7] Fixes for server-side xdr_stream overhaul

[PATCH v2 0/7] Fixes for server-side xdr_stream overhaul

[Chuck Lever]

I've gotten push-back on the idea of rejecting RPC messages where
the RPC record size is larger than the RPC message itself. Therefore
that concept has been dropped from this series.

I've now been able to reproduce, exactly as it was described, a
recently-reported problem with READDIR handling. I've fixed that and
also determined that no other legacy NFS operations appear to be
vulnerable to this particular issue (within the Linux NFS server).


Changes since v1:
- Dropped the xdr_buf_length() helper
- Replaced 7/7 with patch that cleans up an unneeded use of xdr_buf::len
- Dropped the checks for oversized RPC records
- Fixed narrow problem with NFSv2 and NFSv3 READDIR processing

---

Chuck Lever (7):
      SUNRPC: Fix svcxdr_init_decode's end-of-buffer calculation
      SUNRPC: Fix svcxdr_init_encode's buflen calculation
      NFSD: Protect against READDIR send buffer overflow
      NFSD: Use xdr_inline_decode() to decode NFSv3 symlinks
      NFSD: Clean up WRITE arg decoders
      SUNRPC: Fix typo in xdr_buf_subsegment's kdoc comment
      NFSD: Clean up nfs4svc_encode_compoundres()


 fs/nfsd/nfs3proc.c         |  5 ++---
 fs/nfsd/nfs3xdr.c          | 18 ++++--------------
 fs/nfsd/nfs4xdr.c          |  4 ----
 fs/nfsd/nfsproc.c          |  5 ++---
 fs/nfsd/nfsxdr.c           |  4 +---
 include/linux/sunrpc/svc.h | 19 +++++++++++++++----
 net/sunrpc/xdr.c           |  2 +-
 7 files changed, 25 insertions(+), 32 deletions(-)

--
Chuck Lever

[PATCH v2 1/7] SUNRPC: Fix svcxdr_init_decode's end-of-buffer calculation

[Chuck Lever]

Ensure that stream-based argument decoding can't go past the actual
end of the receive buffer. xdr_init_decode's calculation of the
value of xdr->end over-estimates the end of the buffer because the
Linux kernel RPC server code does not remove the size of the RPC
header from rqstp->rq_arg before calling the upper layer's
dispatcher.

The server-side still uses the svc_getnl() macros to decode the
RPC call header. These macros reduce the length of the head iov
but do not update the total length of the message in the buffer
(buf->len).

A proper fix for this would be to replace the use of svc_getnl() and
friends in the RPC header decoder, but that would be a large and
invasive change that would be difficult to backport.

Fixes: 5191955d6fc6 ("SUNRPC: Prepare for xdr_stream-style decoding on the server-side")
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 include/linux/sunrpc/svc.h |   17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index daecb009c05b..5a830b66f059 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -544,16 +544,27 @@ static inline void svc_reserve_auth(struct svc_rqst *rqstp, int space)
 }
 
 /**
- * svcxdr_init_decode - Prepare an xdr_stream for svc Call decoding
+ * svcxdr_init_decode - Prepare an xdr_stream for Call decoding
  * @rqstp: controlling server RPC transaction context
  *
+ * This function currently assumes the RPC header in rq_arg has
+ * already been decoded. Upon return, xdr->p points to the
+ * location of the upper layer header.
  */
 static inline void svcxdr_init_decode(struct svc_rqst *rqstp)
 {
 	struct xdr_stream *xdr = &rqstp->rq_arg_stream;
-	struct kvec *argv = rqstp->rq_arg.head;
+	struct xdr_buf *buf = &rqstp->rq_arg;
+	struct kvec *argv = buf->head;
 
-	xdr_init_decode(xdr, &rqstp->rq_arg, argv->iov_base, NULL);
+	/*
+	 * svc_getnl() and friends do not keep the xdr_buf's ::len
+	 * field up to date. Refresh that field before initializing
+	 * the argument decoding stream.
+	 */
+	buf->len = buf->head->iov_len + buf->page_len + buf->tail->iov_len;
+
+	xdr_init_decode(xdr, buf, argv->iov_base, NULL);
 	xdr_set_scratch_page(xdr, rqstp->rq_scratch_page);
 }
 

[PATCH v2 2/7] SUNRPC: Fix svcxdr_init_encode's buflen calculation

[Chuck Lever]

Commit 2825a7f90753 ("nfsd4: allow encoding across page boundaries")
added an explicit computation of the remaining length in the rq_res
XDR buffer.

The computation appears to suffer from an "off-by-one" bug. Because
buflen is too large by one page, XDR encoding can run off the end of
the send buffer by eventually trying to use the struct page address
in rq_page_end, which always contains NULL.

Fixes: bddfdbcddbe2 ("NFSD: Extract the svcxdr_init_encode() helper")
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 include/linux/sunrpc/svc.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 5a830b66f059..0ca8a8ffb47e 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -587,7 +587,7 @@ static inline void svcxdr_init_encode(struct svc_rqst *rqstp)
 	xdr->end = resv->iov_base + PAGE_SIZE - rqstp->rq_auth_slack;
 	buf->len = resv->iov_len;
 	xdr->page_ptr = buf->pages - 1;
-	buf->buflen = PAGE_SIZE * (1 + rqstp->rq_page_end - buf->pages);
+	buf->buflen = PAGE_SIZE * (rqstp->rq_page_end - buf->pages);
 	buf->buflen -= rqstp->rq_auth_slack;
 	xdr->rqst = NULL;
 }

[PATCH v2 3/7] NFSD: Protect against READDIR send buffer overflow

[Chuck Lever]

For many years, NFSD has conserved the number of pages held by
each nfsd thread by combining the RPC receive and send buffers
into a single array of pages. The dividing line between the
receive and send buffer is pointed to by svc_rqst::rq_respages.

Thus the send buffer shrinks when the received RPC record
containing the RPC Call is large.

nfsd3_init_dirlist_pages() needs to account for the space in the
svc_rqst::rq_pages array already consumed by the RPC receive buffer.
Otherwise READDIR reply encoding can wander off the end of the page
array.

Thanks to Aleksi Illikainen and Kari Hulkko for discovering this
issue.

Reported-by: Ben Ronallo <Benjamin.Ronallo@synopsys.com>
Fixes: f5dcccd647da ("NFSD: Update the NFSv2 READDIR entry encoder to use struct xdr_stream")
Fixes: 7f87fc2d34d4 ("NFSD: Update NFSv3 READDIR entry encoders to use struct xdr_stream")
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs3proc.c |    5 ++---
 fs/nfsd/nfsproc.c  |    5 ++---
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
index a41cca619338..fab87e9e0b20 100644
--- a/fs/nfsd/nfs3proc.c
+++ b/fs/nfsd/nfs3proc.c
@@ -564,12 +564,11 @@ static void nfsd3_init_dirlist_pages(struct svc_rqst *rqstp,
 	struct xdr_buf *buf = &resp->dirlist;
 	struct xdr_stream *xdr = &resp->xdr;
 
-	count = clamp(count, (u32)(XDR_UNIT * 2), svc_max_payload(rqstp));
-
 	memset(buf, 0, sizeof(*buf));
 
 	/* Reserve room for the NULL ptr & eof flag (-2 words) */
-	buf->buflen = count - XDR_UNIT * 2;
+	buf->buflen = clamp(count, (u32)(XDR_UNIT * 2), rqstp->rq_res.buflen);
+	buf->buflen -= XDR_UNIT * 2;
 	buf->pages = rqstp->rq_next_page;
 	rqstp->rq_next_page += (buf->buflen + PAGE_SIZE - 1) >> PAGE_SHIFT;
 
diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
index 7381972f1677..23c273cb68a9 100644
--- a/fs/nfsd/nfsproc.c
+++ b/fs/nfsd/nfsproc.c
@@ -567,12 +567,11 @@ static void nfsd_init_dirlist_pages(struct svc_rqst *rqstp,
 	struct xdr_buf *buf = &resp->dirlist;
 	struct xdr_stream *xdr = &resp->xdr;
 
-	count = clamp(count, (u32)(XDR_UNIT * 2), svc_max_payload(rqstp));
-
 	memset(buf, 0, sizeof(*buf));
 
 	/* Reserve room for the NULL ptr & eof flag (-2 words) */
-	buf->buflen = count - XDR_UNIT * 2;
+	buf->buflen = clamp(count, (u32)(XDR_UNIT * 2), rqstp->rq_res.buflen);
+	buf->buflen -= XDR_UNIT * 2;
 	buf->pages = rqstp->rq_next_page;
 	rqstp->rq_next_page++;
 

[PATCH v2 4/7] NFSD: Use xdr_inline_decode() to decode NFSv3 symlinks

[Chuck Lever]

Replace the check for buffer over/underflow with a helper that is
commonly used for this purpose. The helper also sets xdr->nwords
correctly after successfully linearizing the symlink argument into
the stream's scratch buffer.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs3xdr.c |   14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
index 0293b8d65f10..71e32cf28885 100644
--- a/fs/nfsd/nfs3xdr.c
+++ b/fs/nfsd/nfs3xdr.c
@@ -616,8 +616,6 @@ nfs3svc_decode_symlinkargs(struct svc_rqst *rqstp, struct xdr_stream *xdr)
 {
 	struct nfsd3_symlinkargs *args = rqstp->rq_argp;
 	struct kvec *head = rqstp->rq_arg.head;
-	struct kvec *tail = rqstp->rq_arg.tail;
-	size_t remaining;
 
 	if (!svcxdr_decode_diropargs3(xdr, &args->ffh, &args->fname, &args->flen))
 		return false;
@@ -626,16 +624,10 @@ nfs3svc_decode_symlinkargs(struct svc_rqst *rqstp, struct xdr_stream *xdr)
 	if (xdr_stream_decode_u32(xdr, &args->tlen) < 0)
 		return false;
 
-	/* request sanity */
-	remaining = head->iov_len + rqstp->rq_arg.page_len + tail->iov_len;
-	remaining -= xdr_stream_pos(xdr);
-	if (remaining < xdr_align_size(args->tlen))
-		return false;
-
-	args->first.iov_base = xdr->p;
+	/* symlink_data */
 	args->first.iov_len = head->iov_len - xdr_stream_pos(xdr);
-
-	return true;
+	args->first.iov_base = xdr_inline_decode(xdr, args->tlen);
+	return args->first.iov_base != NULL;
 }
 
 bool

[PATCH v2 5/7] NFSD: Clean up WRITE arg decoders

[Chuck Lever]

xdr_stream_subsegment() already returns a boolean value.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs3xdr.c |    4 +---
 fs/nfsd/nfsxdr.c  |    4 +---
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
index 71e32cf28885..3308dd671ef0 100644
--- a/fs/nfsd/nfs3xdr.c
+++ b/fs/nfsd/nfs3xdr.c
@@ -571,10 +571,8 @@ nfs3svc_decode_writeargs(struct svc_rqst *rqstp, struct xdr_stream *xdr)
 		args->count = max_blocksize;
 		args->len = max_blocksize;
 	}
-	if (!xdr_stream_subsegment(xdr, &args->payload, args->count))
-		return false;
 
-	return true;
+	return xdr_stream_subsegment(xdr, &args->payload, args->count);
 }
 
 bool
diff --git a/fs/nfsd/nfsxdr.c b/fs/nfsd/nfsxdr.c
index aba8520b4b8b..caf6355b18fa 100644
--- a/fs/nfsd/nfsxdr.c
+++ b/fs/nfsd/nfsxdr.c
@@ -338,10 +338,8 @@ nfssvc_decode_writeargs(struct svc_rqst *rqstp, struct xdr_stream *xdr)
 		return false;
 	if (args->len > NFSSVC_MAXBLKSIZE_V2)
 		return false;
-	if (!xdr_stream_subsegment(xdr, &args->payload, args->len))
-		return false;
 
-	return true;
+	return xdr_stream_subsegment(xdr, &args->payload, args->len);
 }
 
 bool

[PATCH v2 6/7] SUNRPC: Fix typo in xdr_buf_subsegment's kdoc comment

[Chuck Lever]

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/sunrpc/xdr.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
index 482586c23fdd..8ad637ca703e 100644
--- a/net/sunrpc/xdr.c
+++ b/net/sunrpc/xdr.c
@@ -1575,7 +1575,7 @@ EXPORT_SYMBOL_GPL(xdr_buf_from_iov);
  *
  * @buf and @subbuf may be pointers to the same struct xdr_buf.
  *
- * Returns -1 if base of length are out of bounds.
+ * Returns -1 if base or length are out of bounds.
  */
 int xdr_buf_subsegment(const struct xdr_buf *buf, struct xdr_buf *subbuf,
 		       unsigned int base, unsigned int len)

[PATCH v2 7/7] NFSD: Clean up nfs4svc_encode_compoundres()

[Chuck Lever]

In today's Linux NFS server implementation, the NFS dispatcher
initializes each XDR result stream, and the NFSv4 .pc_func and
.pc_encode methods all use xdr_stream-based encoding. This keeps
rq_res.len automatically updated. There is no longer a need for
the WARN_ON_ONCE() check in nfs4svc_encode_compoundres().

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs4xdr.c |    4 ----
 1 file changed, 4 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 1e9690a061ec..af51e2a8ceb7 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -5423,12 +5423,8 @@ bool
 nfs4svc_encode_compoundres(struct svc_rqst *rqstp, struct xdr_stream *xdr)
 {
 	struct nfsd4_compoundres *resp = rqstp->rq_resp;
-	struct xdr_buf *buf = xdr->buf;
 	__be32 *p;
 
-	WARN_ON_ONCE(buf->len != buf->head[0].iov_len + buf->page_len +
-				 buf->tail[0].iov_len);
-
 	/*
 	 * Send buffer space for the following items is reserved
 	 * at the top of nfsd4_proc_compound().

Re: [PATCH v2 1/7] SUNRPC: Fix svcxdr_init_decode's end-of-buffer calculation

[Jeff Layton]

On Sun, 2022-08-28 at 14:50 -0400, Chuck Lever wrote:
> Ensure that stream-based argument decoding can't go past the actual
> end of the receive buffer. xdr_init_decode's calculation of the
> value of xdr->end over-estimates the end of the buffer because the
> Linux kernel RPC server code does not remove the size of the RPC
> header from rqstp->rq_arg before calling the upper layer's
> dispatcher.
> 
> The server-side still uses the svc_getnl() macros to decode the
> RPC call header. These macros reduce the length of the head iov
> but do not update the total length of the message in the buffer
> (buf->len).
> 
> A proper fix for this would be to replace the use of svc_getnl() and
> friends in the RPC header decoder, but that would be a large and
> invasive change that would be difficult to backport.
> 
> Fixes: 5191955d6fc6 ("SUNRPC: Prepare for xdr_stream-style decoding on the server-side")
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  include/linux/sunrpc/svc.h |   17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index daecb009c05b..5a830b66f059 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -544,16 +544,27 @@ static inline void svc_reserve_auth(struct svc_rqst *rqstp, int space)
>  }
>  
>  /**
> - * svcxdr_init_decode - Prepare an xdr_stream for svc Call decoding
> + * svcxdr_init_decode - Prepare an xdr_stream for Call decoding
>   * @rqstp: controlling server RPC transaction context
>   *
> + * This function currently assumes the RPC header in rq_arg has
> + * already been decoded. Upon return, xdr->p points to the
> + * location of the upper layer header.

nit: "upper layer header" is a bit nebulous here. Maybe "points to the
start of the RPC program header" ?

>   */
>  static inline void svcxdr_init_decode(struct svc_rqst *rqstp)
>  {
>  	struct xdr_stream *xdr = &rqstp->rq_arg_stream;
> -	struct kvec *argv = rqstp->rq_arg.head;
> +	struct xdr_buf *buf = &rqstp->rq_arg;
> +	struct kvec *argv = buf->head;
>  
> -	xdr_init_decode(xdr, &rqstp->rq_arg, argv->iov_base, NULL);
> +	/*
> +	 * svc_getnl() and friends do not keep the xdr_buf's ::len
> +	 * field up to date. Refresh that field before initializing
> +	 * the argument decoding stream.
> +	 */
> +	buf->len = buf->head->iov_len + buf->page_len + buf->tail->iov_len;
> +
> +	xdr_init_decode(xdr, buf, argv->iov_base, NULL);
>  	xdr_set_scratch_page(xdr, rqstp->rq_scratch_page);
>  }
>  
> 
> 

Patch looks fine. I do wish this code were less confusing with length
handing though I'm not sure how to approach cleaning that up.

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 2/7] SUNRPC: Fix svcxdr_init_encode's buflen calculation

[Jeff Layton]

On Sun, 2022-08-28 at 14:50 -0400, Chuck Lever wrote:
> Commit 2825a7f90753 ("nfsd4: allow encoding across page boundaries")
> added an explicit computation of the remaining length in the rq_res
> XDR buffer.
> 
> The computation appears to suffer from an "off-by-one" bug. Because
> buflen is too large by one page, XDR encoding can run off the end of
> the send buffer by eventually trying to use the struct page address
> in rq_page_end, which always contains NULL.
> 
> Fixes: bddfdbcddbe2 ("NFSD: Extract the svcxdr_init_encode() helper")
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  include/linux/sunrpc/svc.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index 5a830b66f059..0ca8a8ffb47e 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -587,7 +587,7 @@ static inline void svcxdr_init_encode(struct svc_rqst *rqstp)
>  	xdr->end = resv->iov_base + PAGE_SIZE - rqstp->rq_auth_slack;
>  	buf->len = resv->iov_len;
>  	xdr->page_ptr = buf->pages - 1;
> -	buf->buflen = PAGE_SIZE * (1 + rqstp->rq_page_end - buf->pages);
> +	buf->buflen = PAGE_SIZE * (rqstp->rq_page_end - buf->pages);
>  	buf->buflen -= rqstp->rq_auth_slack;
>  	xdr->rqst = NULL;
>  }
> 
> 

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 3/7] NFSD: Protect against READDIR send buffer overflow

[Jeff Layton]

On Sun, 2022-08-28 at 14:50 -0400, Chuck Lever wrote:
> For many years, NFSD has conserved the number of pages held by
> each nfsd thread by combining the RPC receive and send buffers
> into a single array of pages. The dividing line between the
> receive and send buffer is pointed to by svc_rqst::rq_respages.
> 

nit: Given that you don't look at rq_respages in the patch below, the
previous sentence is not particularly relevant. It might be better to
just explain that rq_res describes the part of the array that is the
response buffer, so we want to consult it for the max length.

> Thus the send buffer shrinks when the received RPC record
> containing the RPC Call is large.
> 
> nfsd3_init_dirlist_pages() needs to account for the space in the
> svc_rqst::rq_pages array already consumed by the RPC receive buffer.
> Otherwise READDIR reply encoding can wander off the end of the page
> array.
> 
> Thanks to Aleksi Illikainen and Kari Hulkko for discovering this
> issue.
> 
> Reported-by: Ben Ronallo <Benjamin.Ronallo@synopsys.com>
> Fixes: f5dcccd647da ("NFSD: Update the NFSv2 READDIR entry encoder to use struct xdr_stream")
> Fixes: 7f87fc2d34d4 ("NFSD: Update NFSv3 READDIR entry encoders to use struct xdr_stream")
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfsd/nfs3proc.c |    5 ++---
>  fs/nfsd/nfsproc.c  |    5 ++---
>  2 files changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> index a41cca619338..fab87e9e0b20 100644
> --- a/fs/nfsd/nfs3proc.c
> +++ b/fs/nfsd/nfs3proc.c
> @@ -564,12 +564,11 @@ static void nfsd3_init_dirlist_pages(struct svc_rqst *rqstp,
>  	struct xdr_buf *buf = &resp->dirlist;
>  	struct xdr_stream *xdr = &resp->xdr;
>  
> -	count = clamp(count, (u32)(XDR_UNIT * 2), svc_max_payload(rqstp));
> -
>  	memset(buf, 0, sizeof(*buf));
>  
>  	/* Reserve room for the NULL ptr & eof flag (-2 words) */
> -	buf->buflen = count - XDR_UNIT * 2;
> +	buf->buflen = clamp(count, (u32)(XDR_UNIT * 2), rqstp->rq_res.buflen);
> +	buf->buflen -= XDR_UNIT * 2;
>  	buf->pages = rqstp->rq_next_page;
>  	rqstp->rq_next_page += (buf->buflen + PAGE_SIZE - 1) >> PAGE_SHIFT;
>  
> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> index 7381972f1677..23c273cb68a9 100644
> --- a/fs/nfsd/nfsproc.c
> +++ b/fs/nfsd/nfsproc.c
> @@ -567,12 +567,11 @@ static void nfsd_init_dirlist_pages(struct svc_rqst *rqstp,
>  	struct xdr_buf *buf = &resp->dirlist;
>  	struct xdr_stream *xdr = &resp->xdr;
>  
> -	count = clamp(count, (u32)(XDR_UNIT * 2), svc_max_payload(rqstp));
> -
>  	memset(buf, 0, sizeof(*buf));
>  
>  	/* Reserve room for the NULL ptr & eof flag (-2 words) */
> -	buf->buflen = count - XDR_UNIT * 2;
> +	buf->buflen = clamp(count, (u32)(XDR_UNIT * 2), rqstp->rq_res.buflen);
> +	buf->buflen -= XDR_UNIT * 2;
>  	buf->pages = rqstp->rq_next_page;
>  	rqstp->rq_next_page++;
>  
> 
> 

I wonder if a better fix would be to make svc_max_payload take the
already-consumed arg space into account? We'd need to fix up the other
callers of course.

In any case, the patch itself looks fine:

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 4/7] NFSD: Use xdr_inline_decode() to decode NFSv3 symlinks

[Jeff Layton]

On Sun, 2022-08-28 at 14:50 -0400, Chuck Lever wrote:
> Replace the check for buffer over/underflow with a helper that is
> commonly used for this purpose. The helper also sets xdr->nwords
> correctly after successfully linearizing the symlink argument into
> the stream's scratch buffer.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfsd/nfs3xdr.c |   14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
> index 0293b8d65f10..71e32cf28885 100644
> --- a/fs/nfsd/nfs3xdr.c
> +++ b/fs/nfsd/nfs3xdr.c
> @@ -616,8 +616,6 @@ nfs3svc_decode_symlinkargs(struct svc_rqst *rqstp, struct xdr_stream *xdr)
>  {
>  	struct nfsd3_symlinkargs *args = rqstp->rq_argp;
>  	struct kvec *head = rqstp->rq_arg.head;
> -	struct kvec *tail = rqstp->rq_arg.tail;
> -	size_t remaining;
>  
>  	if (!svcxdr_decode_diropargs3(xdr, &args->ffh, &args->fname, &args->flen))
>  		return false;
> @@ -626,16 +624,10 @@ nfs3svc_decode_symlinkargs(struct svc_rqst *rqstp, struct xdr_stream *xdr)
>  	if (xdr_stream_decode_u32(xdr, &args->tlen) < 0)
>  		return false;
>  
> -	/* request sanity */
> -	remaining = head->iov_len + rqstp->rq_arg.page_len + tail->iov_len;
> -	remaining -= xdr_stream_pos(xdr);
> -	if (remaining < xdr_align_size(args->tlen))
> -		return false;
> -
> -	args->first.iov_base = xdr->p;
> +	/* symlink_data */
>  	args->first.iov_len = head->iov_len - xdr_stream_pos(xdr);
> -
> -	return true;
> +	args->first.iov_base = xdr_inline_decode(xdr, args->tlen);
> +	return args->first.iov_base != NULL;
>  }
>  
>  bool
> 
> 

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 1/7] SUNRPC: Fix svcxdr_init_decode's end-of-buffer calculation

[Chuck Lever III]

> On Aug 29, 2022, at 8:48 AM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Sun, 2022-08-28 at 14:50 -0400, Chuck Lever wrote:
>> Ensure that stream-based argument decoding can't go past the actual
>> end of the receive buffer. xdr_init_decode's calculation of the
>> value of xdr->end over-estimates the end of the buffer because the
>> Linux kernel RPC server code does not remove the size of the RPC
>> header from rqstp->rq_arg before calling the upper layer's
>> dispatcher.
>> 
>> The server-side still uses the svc_getnl() macros to decode the
>> RPC call header. These macros reduce the length of the head iov
>> but do not update the total length of the message in the buffer
>> (buf->len).
>> 
>> A proper fix for this would be to replace the use of svc_getnl() and
>> friends in the RPC header decoder, but that would be a large and
>> invasive change that would be difficult to backport.
>> 
>> Fixes: 5191955d6fc6 ("SUNRPC: Prepare for xdr_stream-style decoding on the server-side")
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> include/linux/sunrpc/svc.h |   17 ++++++++++++++---
>> 1 file changed, 14 insertions(+), 3 deletions(-)
>> 
>> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
>> index daecb009c05b..5a830b66f059 100644
>> --- a/include/linux/sunrpc/svc.h
>> +++ b/include/linux/sunrpc/svc.h
>> @@ -544,16 +544,27 @@ static inline void svc_reserve_auth(struct svc_rqst *rqstp, int space)
>> }
>> 
>> /**
>> - * svcxdr_init_decode - Prepare an xdr_stream for svc Call decoding
>> + * svcxdr_init_decode - Prepare an xdr_stream for Call decoding
>>  * @rqstp: controlling server RPC transaction context
>>  *
>> + * This function currently assumes the RPC header in rq_arg has
>> + * already been decoded. Upon return, xdr->p points to the
>> + * location of the upper layer header.
> 
> nit: "upper layer header" is a bit nebulous here. Maybe "points to the
> start of the RPC program header" ?

Hm. I thought "upper layer header" is the exact terminology
that means "NFS or whatever". I understand what you mean by
"RPC program header" but I've never heard that term before.

But I'm open to other suggestions for clarity.


>>  */
>> static inline void svcxdr_init_decode(struct svc_rqst *rqstp)
>> {
>> 	struct xdr_stream *xdr = &rqstp->rq_arg_stream;
>> -	struct kvec *argv = rqstp->rq_arg.head;
>> +	struct xdr_buf *buf = &rqstp->rq_arg;
>> +	struct kvec *argv = buf->head;
>> 
>> -	xdr_init_decode(xdr, &rqstp->rq_arg, argv->iov_base, NULL);
>> +	/*
>> +	 * svc_getnl() and friends do not keep the xdr_buf's ::len
>> +	 * field up to date. Refresh that field before initializing
>> +	 * the argument decoding stream.
>> +	 */
>> +	buf->len = buf->head->iov_len + buf->page_len + buf->tail->iov_len;
>> +
>> +	xdr_init_decode(xdr, buf, argv->iov_base, NULL);
>> 	xdr_set_scratch_page(xdr, rqstp->rq_scratch_page);
>> }
>> 
>> 
>> 
> 
> Patch looks fine. I do wish this code were less confusing with length
> handing though I'm not sure how to approach cleaning that up.

The plan is to move the call to svcxdr_init_decode() into
svc_process(), eventually, so that svc_getnl() and friends
can be replaced with xdr_stream helpers which intrinsically
manage the xdr_buf message and buffer length fields.

But that means XDR-related code in server-side RPCSEC GSS has
to be converted to xdr_stream too. That's not a weekend project.


> Reviewed-by: Jeff Layton <jlayton@kernel.org>

Thanks!


--
Chuck Lever

Re: [PATCH v2 5/7] NFSD: Clean up WRITE arg decoders

[Jeff Layton]

On Sun, 2022-08-28 at 14:50 -0400, Chuck Lever wrote:
> xdr_stream_subsegment() already returns a boolean value.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfsd/nfs3xdr.c |    4 +---
>  fs/nfsd/nfsxdr.c  |    4 +---
>  2 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
> index 71e32cf28885..3308dd671ef0 100644
> --- a/fs/nfsd/nfs3xdr.c
> +++ b/fs/nfsd/nfs3xdr.c
> @@ -571,10 +571,8 @@ nfs3svc_decode_writeargs(struct svc_rqst *rqstp, struct xdr_stream *xdr)
>  		args->count = max_blocksize;
>  		args->len = max_blocksize;
>  	}
> -	if (!xdr_stream_subsegment(xdr, &args->payload, args->count))
> -		return false;
>  
> -	return true;
> +	return xdr_stream_subsegment(xdr, &args->payload, args->count);
>  }
>  
>  bool
> diff --git a/fs/nfsd/nfsxdr.c b/fs/nfsd/nfsxdr.c
> index aba8520b4b8b..caf6355b18fa 100644
> --- a/fs/nfsd/nfsxdr.c
> +++ b/fs/nfsd/nfsxdr.c
> @@ -338,10 +338,8 @@ nfssvc_decode_writeargs(struct svc_rqst *rqstp, struct xdr_stream *xdr)
>  		return false;
>  	if (args->len > NFSSVC_MAXBLKSIZE_V2)
>  		return false;
> -	if (!xdr_stream_subsegment(xdr, &args->payload, args->len))
> -		return false;
>  
> -	return true;
> +	return xdr_stream_subsegment(xdr, &args->payload, args->len);
>  }
>  
>  bool
> 
> 

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 6/7] SUNRPC: Fix typo in xdr_buf_subsegment's kdoc comment

[Jeff Layton]

On Sun, 2022-08-28 at 14:50 -0400, Chuck Lever wrote:
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  net/sunrpc/xdr.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> index 482586c23fdd..8ad637ca703e 100644
> --- a/net/sunrpc/xdr.c
> +++ b/net/sunrpc/xdr.c
> @@ -1575,7 +1575,7 @@ EXPORT_SYMBOL_GPL(xdr_buf_from_iov);
>   *
>   * @buf and @subbuf may be pointers to the same struct xdr_buf.
>   *
> - * Returns -1 if base of length are out of bounds.
> + * Returns -1 if base or length are out of bounds.
>   */
>  int xdr_buf_subsegment(const struct xdr_buf *buf, struct xdr_buf *subbuf,
>  		       unsigned int base, unsigned int len)
> 
> 

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 7/7] NFSD: Clean up nfs4svc_encode_compoundres()

[Jeff Layton]

On Sun, 2022-08-28 at 14:51 -0400, Chuck Lever wrote:
> In today's Linux NFS server implementation, the NFS dispatcher
> initializes each XDR result stream, and the NFSv4 .pc_func and
> .pc_encode methods all use xdr_stream-based encoding. This keeps
> rq_res.len automatically updated. There is no longer a need for
> the WARN_ON_ONCE() check in nfs4svc_encode_compoundres().
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfsd/nfs4xdr.c |    4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 1e9690a061ec..af51e2a8ceb7 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -5423,12 +5423,8 @@ bool
>  nfs4svc_encode_compoundres(struct svc_rqst *rqstp, struct xdr_stream *xdr)
>  {
>  	struct nfsd4_compoundres *resp = rqstp->rq_resp;
> -	struct xdr_buf *buf = xdr->buf;
>  	__be32 *p;
>  
> -	WARN_ON_ONCE(buf->len != buf->head[0].iov_len + buf->page_len +
> -				 buf->tail[0].iov_len);
> -
>  	/*
>  	 * Send buffer space for the following items is reserved
>  	 * at the top of nfsd4_proc_compound().
> 
> 

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 3/7] NFSD: Protect against READDIR send buffer overflow

[Chuck Lever III]

> On Aug 29, 2022, at 9:43 AM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Sun, 2022-08-28 at 14:50 -0400, Chuck Lever wrote:
>> For many years, NFSD has conserved the number of pages held by
>> each nfsd thread by combining the RPC receive and send buffers
>> into a single array of pages. The dividing line between the
>> receive and send buffer is pointed to by svc_rqst::rq_respages.
>> 
> 
> nit: Given that you don't look at rq_respages in the patch below, the
> previous sentence is not particularly relevant. It might be better to
> just explain that rq_res describes the part of the array that is the
> response buffer, so we want to consult it for the max length.

Good point.


>> Thus the send buffer shrinks when the received RPC record
>> containing the RPC Call is large.
>> 
>> nfsd3_init_dirlist_pages() needs to account for the space in the
>> svc_rqst::rq_pages array already consumed by the RPC receive buffer.
>> Otherwise READDIR reply encoding can wander off the end of the page
>> array.
>> 
>> Thanks to Aleksi Illikainen and Kari Hulkko for discovering this
>> issue.
>> 
>> Reported-by: Ben Ronallo <Benjamin.Ronallo@synopsys.com>
>> Fixes: f5dcccd647da ("NFSD: Update the NFSv2 READDIR entry encoder to use struct xdr_stream")
>> Fixes: 7f87fc2d34d4 ("NFSD: Update NFSv3 READDIR entry encoders to use struct xdr_stream")
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> fs/nfsd/nfs3proc.c |    5 ++---
>> fs/nfsd/nfsproc.c  |    5 ++---
>> 2 files changed, 4 insertions(+), 6 deletions(-)
>> 
>> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
>> index a41cca619338..fab87e9e0b20 100644
>> --- a/fs/nfsd/nfs3proc.c
>> +++ b/fs/nfsd/nfs3proc.c
>> @@ -564,12 +564,11 @@ static void nfsd3_init_dirlist_pages(struct svc_rqst *rqstp,
>> 	struct xdr_buf *buf = &resp->dirlist;
>> 	struct xdr_stream *xdr = &resp->xdr;
>> 
>> -	count = clamp(count, (u32)(XDR_UNIT * 2), svc_max_payload(rqstp));
>> -
>> 	memset(buf, 0, sizeof(*buf));
>> 
>> 	/* Reserve room for the NULL ptr & eof flag (-2 words) */
>> -	buf->buflen = count - XDR_UNIT * 2;
>> +	buf->buflen = clamp(count, (u32)(XDR_UNIT * 2), rqstp->rq_res.buflen);
>> +	buf->buflen -= XDR_UNIT * 2;
>> 	buf->pages = rqstp->rq_next_page;
>> 	rqstp->rq_next_page += (buf->buflen + PAGE_SIZE - 1) >> PAGE_SHIFT;
>> 
>> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
>> index 7381972f1677..23c273cb68a9 100644
>> --- a/fs/nfsd/nfsproc.c
>> +++ b/fs/nfsd/nfsproc.c
>> @@ -567,12 +567,11 @@ static void nfsd_init_dirlist_pages(struct svc_rqst *rqstp,
>> 	struct xdr_buf *buf = &resp->dirlist;
>> 	struct xdr_stream *xdr = &resp->xdr;
>> 
>> -	count = clamp(count, (u32)(XDR_UNIT * 2), svc_max_payload(rqstp));
>> -
>> 	memset(buf, 0, sizeof(*buf));
>> 
>> 	/* Reserve room for the NULL ptr & eof flag (-2 words) */
>> -	buf->buflen = count - XDR_UNIT * 2;
>> +	buf->buflen = clamp(count, (u32)(XDR_UNIT * 2), rqstp->rq_res.buflen);
>> +	buf->buflen -= XDR_UNIT * 2;
>> 	buf->pages = rqstp->rq_next_page;
>> 	rqstp->rq_next_page++;
>> 
>> 
>> 
> 
> I wonder if a better fix would be to make svc_max_payload take the
> already-consumed arg space into account? We'd need to fix up the other
> callers of course.

svc_max_payload() is used in places where the server's maximum
payload value is desired and in other places where the request's
maximum payload value is desired (as in this case). We'd have to
sort these cases.

But, now that I'm looking at svc_max_payload() call sites, it
does appear that some of these will fall prey to the same bug.
Eg, nfsd3_proc_read().

So, let me do that audit and redrive the series.


> In any case, the patch itself looks fine:
> 
> Reviewed-by: Jeff Layton <jlayton@kernel.org>

--
Chuck Lever