[PATCH v1 1/7] SUNRPC: Fix end-of-buffer calculation

[PATCH v1 1/7] SUNRPC: Fix 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 |   14 +++++++++++---
 include/linux/sunrpc/xdr.h |   12 ++++++++++++
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index daecb009c05b..494375313a6f 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -544,16 +544,24 @@ 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);
+	/* Reset the argument buffer's length, now that the RPC header
+	 * has been decoded. */
+	buf->len = xdr_buf_length(buf);
+
+	xdr_init_decode(xdr, buf, argv->iov_base, NULL);
 	xdr_set_scratch_page(xdr, rqstp->rq_scratch_page);
 }
 
diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
index 69175029abbb..f6bb215d4029 100644
--- a/include/linux/sunrpc/xdr.h
+++ b/include/linux/sunrpc/xdr.h
@@ -83,6 +83,18 @@ xdr_buf_init(struct xdr_buf *buf, void *start, size_t len)
 	buf->buflen = len;
 }
 
+/**
+ * xdr_buf_length - Return the summed length of @buf's sub-buffers
+ * @buf: an instantiated xdr_buf
+ *
+ * Returns the sum of the lengths of the head kvec, the tail kvec,
+ * and the page array.
+ */
+static inline unsigned int xdr_buf_length(const struct xdr_buf *buf)
+{
+	return buf->head->iov_len + buf->page_len + buf->tail->iov_len;
+}
+
 /*
  * pre-xdr'ed macros.
  */

[PATCH v1 2/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 v1 3/7] NFSD: Check for junk after RPC Call messages

[Chuck Lever]

The current RPC server code allows incoming RPC messages up to about
a megabyte in size. For TCP, this is based on the size value
contained in the RPC record marker.

Currently, NFSD ignores anything in the message that is past the end
of the encoded RPC Call message. A very large RPC message can arrive
with just an NFSv3 LOOKUP operation in it, and NFSD ignores the rest
of the message until the next RPC fragment in the TCP stream.

That ignored data still consumes pages in the svc_rqst's page array,
however. The current arrangement is that each svc_rqst gets about
260 pages, assuming that all supported NFS operations will never
require more than a total of 260 pages to decode a Call message
and construct its corresponding Reply message.

A clever attacker can add garbage at the end of a READ-like request,
which generally has a small Call message and a potentially large
Reply message with a payload . That makes both the Call message and
the Reply message large, and runs the svc_rqst out of pages. At the
very least, this can result in a short or empty READ or READDIR
result.

So, let's teach NFSD to look for such shenanigans and reject any
Call where the incoming RPC frame has content remaining in the
receive buffer after NFSD has decoded all of the Call arguments.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfssvc.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 4bb5baa17040..5face047ce1a 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -1027,6 +1027,7 @@ nfsd(void *vrqstp)
 int nfsd_dispatch(struct svc_rqst *rqstp, __be32 *statp)
 {
 	const struct svc_procedure *proc = rqstp->rq_procinfo;
+	struct xdr_stream *xdr = &rqstp->rq_arg_stream;
 
 	/*
 	 * Give the xdr decoder a chance to change this if it wants
@@ -1035,7 +1036,9 @@ int nfsd_dispatch(struct svc_rqst *rqstp, __be32 *statp)
 	rqstp->rq_cachetype = proc->pc_cachetype;
 
 	svcxdr_init_decode(rqstp);
-	if (!proc->pc_decode(rqstp, &rqstp->rq_arg_stream))
+	if (!proc->pc_decode(rqstp, xdr))
+		goto out_decode_err;
+	if (xdr_stream_remaining(xdr))
 		goto out_decode_err;
 
 	switch (nfsd_cache_lookup(rqstp)) {

[PATCH v1 4/7] lockd: Check for junk after RPC Call messages

[Chuck Lever]

The current RPC server code allows incoming RPC messages up to about
a megabyte in size. For TCP, this is based on the size value
contained in the RPC record marker.

Currently, lockd ignores anything in the message that is past the end
of the encoded RPC Call message. A very large RPC message can arrive
with just an NLM LOCK operation in it, and lockd ignores the rest of
the message until the next RPC fragment in the TCP stream.

That ignored data still consumes pages in the svc_rqst's page array,
however. The current arrangement is that each svc_rqst gets about
260 pages, assuming that all supported NLM operations will never
require more than a total of 260 pages to decode a Call message
and construct its corresponding Reply message.

A clever attacker can add garbage at the end of an RPC Call message.
At the least, it can result in a short or empty NLM result.

So, let's teach lockd to look for such shenanigans and reject any
Call where the incoming RPC frame has content remaining in the
receive buffer after lockd has decoded the Call arguments.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/lockd/svc.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index 59ef8a1f843f..80b3f1a006f6 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -694,9 +694,12 @@ module_exit(exit_nlm);
 static int nlmsvc_dispatch(struct svc_rqst *rqstp, __be32 *statp)
 {
 	const struct svc_procedure *procp = rqstp->rq_procinfo;
+	struct xdr_stream *xdr = &rqstp->rq_arg_stream;
 
 	svcxdr_init_decode(rqstp);
-	if (!procp->pc_decode(rqstp, &rqstp->rq_arg_stream))
+	if (!procp->pc_decode(rqstp, xdr))
+		goto out_decode_err;
+	if (xdr_stream_remaining(xdr))
 		goto out_decode_err;
 
 	*statp = procp->pc_func(rqstp);

[PATCH v1 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 v1 6/7] SUNRPC: Clean up 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 v1 7/7] SUNRPC: Use the new xdr_buf_length() helper

[Chuck Lever]

Clean up.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs4xdr.c |    3 +--
 net/sunrpc/xdr.c  |    5 ++---
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 1e9690a061ec..26d005c5ec10 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -5426,8 +5426,7 @@ nfs4svc_encode_compoundres(struct svc_rqst *rqstp, struct xdr_stream *xdr)
 	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);
+	WARN_ON_ONCE(buf->len != xdr_buf_length(buf));
 
 	/*
 	 * Send buffer space for the following items is reserved
diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
index 8ad637ca703e..f77a7d98b294 100644
--- a/net/sunrpc/xdr.c
+++ b/net/sunrpc/xdr.c
@@ -508,9 +508,8 @@ static unsigned int xdr_buf_pages_fill_sparse(const struct xdr_buf *buf,
 
 static void xdr_buf_try_expand(struct xdr_buf *buf, unsigned int len)
 {
-	struct kvec *head = buf->head;
 	struct kvec *tail = buf->tail;
-	unsigned int sum = head->iov_len + buf->page_len + tail->iov_len;
+	unsigned int sum = xdr_buf_length(buf);
 	unsigned int free_space, newlen;
 
 	if (sum > buf->len) {
@@ -2060,7 +2059,7 @@ int xdr_encode_array2(const struct xdr_buf *buf, unsigned int base,
 		      struct xdr_array2_desc *desc)
 {
 	if ((unsigned long) base + 4 + desc->array_len * desc->elem_size >
-	    buf->head->iov_len + buf->page_len + buf->tail->iov_len)
+	    xdr_buf_length(buf))
 		return -EINVAL;
 
 	return xdr_xcode_array2(buf, base, desc, 1);

Re: [PATCH v1 1/7] SUNRPC: Fix end-of-buffer calculation

[Trond Myklebust]

On Tue, 2022-08-23 at 17:00 -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 |   14 +++++++++++---
>  include/linux/sunrpc/xdr.h |   12 ++++++++++++
>  2 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index daecb009c05b..494375313a6f 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -544,16 +544,24 @@ 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);
> +       /* Reset the argument buffer's length, now that the RPC
> header
> +        * has been decoded. */
> +       buf->len = xdr_buf_length(buf);
> +
> +       xdr_init_decode(xdr, buf, argv->iov_base, NULL);
>         xdr_set_scratch_page(xdr, rqstp->rq_scratch_page);
>  }
>  
> diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
> index 69175029abbb..f6bb215d4029 100644
> --- a/include/linux/sunrpc/xdr.h
> +++ b/include/linux/sunrpc/xdr.h
> @@ -83,6 +83,18 @@ xdr_buf_init(struct xdr_buf *buf, void *start,
> size_t len)
>         buf->buflen = len;
>  }
>  
> +/**
> + * xdr_buf_length - Return the summed length of @buf's sub-buffers
> + * @buf: an instantiated xdr_buf
> + *
> + * Returns the sum of the lengths of the head kvec, the tail kvec,
> + * and the page array.
> + */
> +static inline unsigned int xdr_buf_length(const struct xdr_buf *buf)
> +{
> +       return buf->head->iov_len + buf->page_len + buf->tail-
> >iov_len;
> +}
> +

NACK. This function is neither needed nor wanted for the client code,
which already does the right thing w.r.t. maintaining an authoritative
buf->len.

If you need this helper, then stuff under a server-specific mattress
where developers looking for client functionality can't find it.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH v1 1/7] SUNRPC: Fix end-of-buffer calculation

[Chuck Lever III]

> On Aug 23, 2022, at 5:52 PM, Trond Myklebust <trondmy@hammerspace.com> wrote:
> 
> On Tue, 2022-08-23 at 17:00 -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 |   14 +++++++++++---
>>  include/linux/sunrpc/xdr.h |   12 ++++++++++++
>>  2 files changed, 23 insertions(+), 3 deletions(-)
>> 
>> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
>> index daecb009c05b..494375313a6f 100644
>> --- a/include/linux/sunrpc/svc.h
>> +++ b/include/linux/sunrpc/svc.h
>> @@ -544,16 +544,24 @@ 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);
>> +       /* Reset the argument buffer's length, now that the RPC
>> header
>> +        * has been decoded. */
>> +       buf->len = xdr_buf_length(buf);
>> +
>> +       xdr_init_decode(xdr, buf, argv->iov_base, NULL);
>>         xdr_set_scratch_page(xdr, rqstp->rq_scratch_page);
>>  }
>>  
>> diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
>> index 69175029abbb..f6bb215d4029 100644
>> --- a/include/linux/sunrpc/xdr.h
>> +++ b/include/linux/sunrpc/xdr.h
>> @@ -83,6 +83,18 @@ xdr_buf_init(struct xdr_buf *buf, void *start,
>> size_t len)
>>         buf->buflen = len;
>>  }
>>  
>> +/**
>> + * xdr_buf_length - Return the summed length of @buf's sub-buffers
>> + * @buf: an instantiated xdr_buf
>> + *
>> + * Returns the sum of the lengths of the head kvec, the tail kvec,
>> + * and the page array.
>> + */
>> +static inline unsigned int xdr_buf_length(const struct xdr_buf *buf)
>> +{
>> +       return buf->head->iov_len + buf->page_len + buf->tail-
>>> iov_len;
>> +}
>> +
> 
> NACK. This function is neither needed nor wanted for the client code,
> which already does the right thing w.r.t. maintaining an authoritative
> buf->len.

See patch 7/7 in this series, which updates two functions that are part
of client-side call chains.

I'm reading into your objection a little, but I think your long term
goal is to have the XDR layer manage ::len opaquely to RPC consumers
in both the client and the server implementation.

I agree that's a good goal, and yes, eventually I will pull the chain
and replace the use of svc_getnl() and friends with xdr_stream-based
decoding. Just not today.


> If you need this helper, then stuff under a server-specific mattress
> where developers looking for client functionality can't find it.

I don't have to have this helper, it was meant as nothing more than
code de-duplication. I'll remove it and drop 7/7.

Thanks for taking a look!


--
Chuck Lever