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
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 <[email protected]>
---
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;
}
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 | 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);
}
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 <[email protected]>
---
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().
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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
> ---
> 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 <[email protected]>
> On Aug 29, 2022, at 8:48 AM, Jeff Layton <[email protected]> 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 <[email protected]>
>> ---
>> 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 <[email protected]>
Thanks!
--
Chuck Lever
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 <[email protected]>
> ---
> 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 <[email protected]>