During review of the v2 of this series, Jeff suggested looking at
svc_max_payload() call sites for similar issues, and I found some.
I've included fixes for NFSv2 and NFSv3 operations in v3 of this
series.
The NFSv4 stack is different than NFSv2/3, so the simple fixes
proposed here are not appropriate there. For one thing, NFSv4 has
these op_rsize_bop helpers that use svc_max_payload() to estimate
the reply size -- but these are called well before
svcxdr_init_encode() has set rq_res.buflen. I'm still working on
fixes for those (including get/listxattr, getattr, read, readdir,
etc).
Changes since v2:
- Dropped the clean-up patches; will re-post those separately, later
- Added fixes for NFSv3 READ and for NFSv2 READ and READDIR
- Hopefully addressed a crash when @dircount is too large
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 (6):
SUNRPC: Fix svcxdr_init_decode's end-of-buffer calculation
SUNRPC: Fix svcxdr_init_encode's buflen calculation
NFSD: Protect against send buffer overflow in NFSv2 READDIR
NFSD: Protect against send buffer overflow in NFSv3 READDIR
NFSD: Protect against send buffer overflow in NFSv2 READ
NFSD: Protect against send buffer overflow in NFSv3 READ
fs/nfsd/nfs3proc.c | 11 ++++++-----
fs/nfsd/nfsproc.c | 6 +++---
include/linux/sunrpc/svc.h | 19 +++++++++++++++----
3 files changed, 24 insertions(+), 12 deletions(-)
--
Chuck Lever
Since before the git era, 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. This works because there are
no cases where an operation needs a large RPC Call message and a
large RPC Reply at the same time.
Once an RPC Call has been received, svc_process() updates
svc_rqst::rq_res to describe the part of rq_pages that can be
used for constructing the Reply. This means that the send buffer
(rq_res) shrinks when the received RPC record containing the RPC
Call is large.
A client can force this shrinkage on TCP by sending a correctly-
formed RPC Call header contained in an RPC record that is
excessively large. The full maximum payload size cannot be
constructed in that case.
Cc: <[email protected]>
Signed-off-by: Chuck Lever <[email protected]>
---
fs/nfsd/nfsproc.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
index ddb1902c0a18..4b19cc727ea5 100644
--- a/fs/nfsd/nfsproc.c
+++ b/fs/nfsd/nfsproc.c
@@ -185,6 +185,7 @@ nfsd_proc_read(struct svc_rqst *rqstp)
argp->count, argp->offset);
argp->count = min_t(u32, argp->count, NFSSVC_MAXBLKSIZE_V2);
+ argp->count = min_t(u32, argp->count, rqstp->rq_res.buflen);
v = 0;
len = argp->count;
Since before the git era, 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. This works because there are
no cases where an operation needs a large RPC Call message and a
large RPC Reply message at the same time.
Once an RPC Call has been received, svc_process() updates
svc_rqst::rq_res to describe the part of rq_pages that can be
used for constructing the Reply. This means that the send buffer
(rq_res) shrinks when the received RPC record containing the RPC
Call is large.
A client can force this shrinkage on TCP by sending a correctly-
formed RPC Call header contained in an RPC record that is
excessively large. The full maximum payload size cannot be
constructed in that case.
Thanks to Aleksi Illikainen and Kari Hulkko for uncovering this
issue.
Reported-by: Ben Ronallo <[email protected]>
Cc: <[email protected]>
Signed-off-by: Chuck Lever <[email protected]>
---
fs/nfsd/nfs3proc.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
index a41cca619338..7a159785499a 100644
--- a/fs/nfsd/nfs3proc.c
+++ b/fs/nfsd/nfs3proc.c
@@ -563,13 +563,14 @@ 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));
+ unsigned int sendbuf = min_t(unsigned int, rqstp->rq_res.buflen,
+ 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), sendbuf);
+ buf->buflen -= XDR_UNIT * 2;
buf->pages = rqstp->rq_next_page;
rqstp->rq_next_page += (buf->buflen + PAGE_SIZE - 1) >> PAGE_SHIFT;
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")
Reviewed-by: Jeff Layton <[email protected]>
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);
}
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")
Reviewed-by: Jeff Layton <[email protected]>
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;
}
Since before the git era, 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. This works because there are
no cases where an operation needs a large RPC Call message and a
large RPC Reply at the same time.
Once an RPC Call has been received, svc_process() updates
svc_rqst::rq_res to describe the part of rq_pages that can be
used for constructing the Reply. This means that the send buffer
(rq_res) shrinks when the received RPC record containing the RPC
Call is large.
A client can force this shrinkage on TCP by sending a correctly-
formed RPC Call header contained in an RPC record that is
excessively large. The full maximum payload size cannot be
constructed in that case.
Cc: <[email protected]>
Signed-off-by: Chuck Lever <[email protected]>
---
fs/nfsd/nfs3proc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
index 7a159785499a..5b1e771238b3 100644
--- a/fs/nfsd/nfs3proc.c
+++ b/fs/nfsd/nfs3proc.c
@@ -150,7 +150,6 @@ nfsd3_proc_read(struct svc_rqst *rqstp)
{
struct nfsd3_readargs *argp = rqstp->rq_argp;
struct nfsd3_readres *resp = rqstp->rq_resp;
- u32 max_blocksize = svc_max_payload(rqstp);
unsigned int len;
int v;
@@ -159,7 +158,8 @@ nfsd3_proc_read(struct svc_rqst *rqstp)
(unsigned long) argp->count,
(unsigned long long) argp->offset);
- argp->count = min_t(u32, argp->count, max_blocksize);
+ argp->count = min_t(u32, argp->count, svc_max_payload(rqstp));
+ argp->count = min_t(u32, argp->count, rqstp->rq_res.buflen);
if (argp->offset > (u64)OFFSET_MAX)
argp->offset = (u64)OFFSET_MAX;
if (argp->offset + argp->count > (u64)OFFSET_MAX)
Restore the previous limit on the @count argument to prevent a
buffer overflow attack.
Fixes: 53b1119a6e50 ("NFSD: Fix READDIR buffer overflow")
Signed-off-by: Chuck Lever <[email protected]>
---
fs/nfsd/nfsproc.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
index 7381972f1677..ddb1902c0a18 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), (u32)PAGE_SIZE);
+ buf->buflen -= XDR_UNIT * 2;
buf->pages = rqstp->rq_next_page;
rqstp->rq_next_page++;
On Thu, 2022-09-01 at 15:10 -0400, Chuck Lever wrote:
> Restore the previous limit on the @count argument to prevent a
> buffer overflow attack.
>
> Fixes: 53b1119a6e50 ("NFSD: Fix READDIR buffer overflow")
> Signed-off-by: Chuck Lever <[email protected]>
> ---
> fs/nfsd/nfsproc.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> index 7381972f1677..ddb1902c0a18 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), (u32)PAGE_SIZE);
> + buf->buflen -= XDR_UNIT * 2;
> buf->pages = rqstp->rq_next_page;
> rqstp->rq_next_page++;
>
>
>
Reviewed-by: Jeff Layton <[email protected]>
On Thu, 2022-09-01 at 15:10 -0400, Chuck Lever wrote:
> Since before the git era, 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. This works because there are
> no cases where an operation needs a large RPC Call message and a
> large RPC Reply at the same time.
>
> Once an RPC Call has been received, svc_process() updates
> svc_rqst::rq_res to describe the part of rq_pages that can be
> used for constructing the Reply. This means that the send buffer
> (rq_res) shrinks when the received RPC record containing the RPC
> Call is large.
>
> A client can force this shrinkage on TCP by sending a correctly-
> formed RPC Call header contained in an RPC record that is
> excessively large. The full maximum payload size cannot be
> constructed in that case.
>
> Cc: <[email protected]>
> Signed-off-by: Chuck Lever <[email protected]>
> ---
> fs/nfsd/nfsproc.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> index ddb1902c0a18..4b19cc727ea5 100644
> --- a/fs/nfsd/nfsproc.c
> +++ b/fs/nfsd/nfsproc.c
> @@ -185,6 +185,7 @@ nfsd_proc_read(struct svc_rqst *rqstp)
> argp->count, argp->offset);
>
> argp->count = min_t(u32, argp->count, NFSSVC_MAXBLKSIZE_V2);
> + argp->count = min_t(u32, argp->count, rqstp->rq_res.buflen);
>
> v = 0;
> len = argp->count;
>
>
Reviewed-by: Jeff Layton <[email protected]>
On Thu, 2022-09-01 at 15:10 -0400, Chuck Lever wrote:
> Since before the git era, 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. This works because there are
> no cases where an operation needs a large RPC Call message and a
> large RPC Reply message at the same time.
>
> Once an RPC Call has been received, svc_process() updates
> svc_rqst::rq_res to describe the part of rq_pages that can be
> used for constructing the Reply. This means that the send buffer
> (rq_res) shrinks when the received RPC record containing the RPC
> Call is large.
>
> A client can force this shrinkage on TCP by sending a correctly-
> formed RPC Call header contained in an RPC record that is
> excessively large. The full maximum payload size cannot be
> constructed in that case.
>
> Thanks to Aleksi Illikainen and Kari Hulkko for uncovering this
> issue.
>
> Reported-by: Ben Ronallo <[email protected]>
> Cc: <[email protected]>
> Signed-off-by: Chuck Lever <[email protected]>
> ---
> fs/nfsd/nfs3proc.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> index a41cca619338..7a159785499a 100644
> --- a/fs/nfsd/nfs3proc.c
> +++ b/fs/nfsd/nfs3proc.c
> @@ -563,13 +563,14 @@ 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));
> + unsigned int sendbuf = min_t(unsigned int, rqstp->rq_res.buflen,
> + 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), sendbuf);
> + buf->buflen -= XDR_UNIT * 2;
> buf->pages = rqstp->rq_next_page;
> rqstp->rq_next_page += (buf->buflen + PAGE_SIZE - 1) >> PAGE_SHIFT;
>
>
>
Reviewed-by: Jeff Layton <[email protected]>
On Thu, 2022-09-01 at 15:10 -0400, Chuck Lever wrote:
> Since before the git era, 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. This works because there are
> no cases where an operation needs a large RPC Call message and a
> large RPC Reply at the same time.
>
> Once an RPC Call has been received, svc_process() updates
> svc_rqst::rq_res to describe the part of rq_pages that can be
> used for constructing the Reply. This means that the send buffer
> (rq_res) shrinks when the received RPC record containing the RPC
> Call is large.
>
> A client can force this shrinkage on TCP by sending a correctly-
> formed RPC Call header contained in an RPC record that is
> excessively large. The full maximum payload size cannot be
> constructed in that case.
>
> Cc: <[email protected]>
> Signed-off-by: Chuck Lever <[email protected]>
> ---
> fs/nfsd/nfs3proc.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> index 7a159785499a..5b1e771238b3 100644
> --- a/fs/nfsd/nfs3proc.c
> +++ b/fs/nfsd/nfs3proc.c
> @@ -150,7 +150,6 @@ nfsd3_proc_read(struct svc_rqst *rqstp)
> {
> struct nfsd3_readargs *argp = rqstp->rq_argp;
> struct nfsd3_readres *resp = rqstp->rq_resp;
> - u32 max_blocksize = svc_max_payload(rqstp);
> unsigned int len;
> int v;
>
> @@ -159,7 +158,8 @@ nfsd3_proc_read(struct svc_rqst *rqstp)
> (unsigned long) argp->count,
> (unsigned long long) argp->offset);
>
> - argp->count = min_t(u32, argp->count, max_blocksize);
> + argp->count = min_t(u32, argp->count, svc_max_payload(rqstp));
> + argp->count = min_t(u32, argp->count, rqstp->rq_res.buflen);
> if (argp->offset > (u64)OFFSET_MAX)
> argp->offset = (u64)OFFSET_MAX;
> if (argp->offset + argp->count > (u64)OFFSET_MAX)
>
>
Reviewed-by: Jeff Layton <[email protected]>