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 <[email protected]>
---
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.
*/
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 <[email protected]>
---
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)) {
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
[email protected]
> On Aug 23, 2022, at 5:52 PM, Trond Myklebust <[email protected]> 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 <[email protected]>
>> ---
>> 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