* [PATCH v1 0/3] rq_pages bounds checking
@ 2023-03-17 23:01 Chuck Lever
2023-03-17 23:01 ` [PATCH v1 1/3] nfsd: don't replace page in rq_pages if it's a continuation of last page Chuck Lever
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: Chuck Lever @ 2023-03-17 23:01 UTC (permalink / raw)
To: linux-nfs; +Cc: viro, dcritch, d.lesca
A slightly modified take on Jeff's earlier patches, tested with
both NFSv3 and NFSv4.1 via simple fault injection in
svc_rqst_replace_page().
In general I'm in favor of more rq_pages bounds checking by
replacing direct modification of the rq_respages and rq_next_page
fields with accessor functions.
---
Chuck Lever (2):
SUNRPC: add bounds checking to svc_rqst_replace_page
NFSD: Watch for rq_pages bounds checking errors in nfsd_splice_actor()
Jeff Layton (1):
nfsd: don't replace page in rq_pages if it's a continuation of last page
fs/nfsd/vfs.c | 15 +++++++++++++--
include/linux/sunrpc/svc.h | 2 +-
include/trace/events/sunrpc.h | 25 +++++++++++++++++++++++++
net/sunrpc/svc.c | 15 ++++++++++++++-
4 files changed, 53 insertions(+), 4 deletions(-)
--
Chuck Lever
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v1 1/3] nfsd: don't replace page in rq_pages if it's a continuation of last page
2023-03-17 23:01 [PATCH v1 0/3] rq_pages bounds checking Chuck Lever
@ 2023-03-17 23:01 ` Chuck Lever
2023-03-17 23:01 ` [PATCH v1 2/3] SUNRPC: add bounds checking to svc_rqst_replace_page Chuck Lever
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Chuck Lever @ 2023-03-17 23:01 UTC (permalink / raw)
To: linux-nfs; +Cc: viro, dcritch, d.lesca
From: Jeff Layton <jlayton@kernel.org>
The splice read calls nfsd_splice_actor to put the pages containing file
data into the svc_rqst->rq_pages array. It's possible however to get a
splice result that only has a partial page at the end, if (e.g.) the
filesystem hands back a short read that doesn't cover the whole page.
nfsd_splice_actor will plop the partial page into its rq_pages array and
return. Then later, when nfsd_splice_actor is called again, the
remainder of the page may end up being filled out. At this point,
nfsd_splice_actor will put the page into the array _again_ corrupting
the reply. If this is done enough times, rq_next_page will overrun the
array and corrupt the trailing fields -- the rq_respages and
rq_next_page pointers themselves.
If we've already added the page to the array in the last pass, don't add
it to the array a second time when dealing with a splice continuation.
This was originally handled properly in nfsd_splice_actor, but commit
91e23b1c3982 ("NFSD: Clean up nfsd_splice_actor()") removed the check
for it.
Fixes: 91e23b1c3982 ("NFSD: Clean up nfsd_splice_actor()")
Cc: Al Viro <viro@zeniv.linux.org.uk>
Reported-by: Dario Lesca <d.lesca@solinos.it>
Tested-by: David Critch <dcritch@redhat.com>
Link: https://bugzilla.redhat.com/show_bug.cgi?id=2150630
Signed-off-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
fs/nfsd/vfs.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 502e1b7742db..5783209f17fc 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -941,8 +941,15 @@ nfsd_splice_actor(struct pipe_inode_info *pipe, struct pipe_buffer *buf,
struct page *last_page;
last_page = page + (offset + sd->len - 1) / PAGE_SIZE;
- for (page += offset / PAGE_SIZE; page <= last_page; page++)
+ for (page += offset / PAGE_SIZE; page <= last_page; page++) {
+ /*
+ * Skip page replacement when extending the contents
+ * of the current page.
+ */
+ if (page == *(rqstp->rq_next_page - 1))
+ continue;
svc_rqst_replace_page(rqstp, page);
+ }
if (rqstp->rq_res.page_len == 0) // first call
rqstp->rq_res.page_base = offset % PAGE_SIZE;
rqstp->rq_res.page_len += sd->len;
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v1 2/3] SUNRPC: add bounds checking to svc_rqst_replace_page
2023-03-17 23:01 [PATCH v1 0/3] rq_pages bounds checking Chuck Lever
2023-03-17 23:01 ` [PATCH v1 1/3] nfsd: don't replace page in rq_pages if it's a continuation of last page Chuck Lever
@ 2023-03-17 23:01 ` Chuck Lever
2023-03-17 23:01 ` [PATCH v1 3/3] NFSD: Watch for rq_pages bounds checking errors in nfsd_splice_actor() Chuck Lever
2023-03-18 10:04 ` [PATCH v1 0/3] rq_pages bounds checking Jeff Layton
3 siblings, 0 replies; 7+ messages in thread
From: Chuck Lever @ 2023-03-17 23:01 UTC (permalink / raw)
To: linux-nfs; +Cc: viro, dcritch, d.lesca
From: Chuck Lever <chuck.lever@oracle.com>
There have been several bugs over the years where the NFSD splice
actor has attempted to write outside the rq_pages array.
Suggested-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
include/linux/sunrpc/svc.h | 2 +-
include/trace/events/sunrpc.h | 25 +++++++++++++++++++++++++
net/sunrpc/svc.c | 15 ++++++++++++++-
3 files changed, 40 insertions(+), 2 deletions(-)
diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 877891536c2f..f5af055280ff 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -422,7 +422,7 @@ struct svc_serv *svc_create(struct svc_program *, unsigned int,
int (*threadfn)(void *data));
struct svc_rqst *svc_rqst_alloc(struct svc_serv *serv,
struct svc_pool *pool, int node);
-void svc_rqst_replace_page(struct svc_rqst *rqstp,
+bool svc_rqst_replace_page(struct svc_rqst *rqstp,
struct page *page);
void svc_rqst_free(struct svc_rqst *);
void svc_exit_thread(struct svc_rqst *);
diff --git a/include/trace/events/sunrpc.h b/include/trace/events/sunrpc.h
index 3ca54536f8f7..5a3bb42e1f50 100644
--- a/include/trace/events/sunrpc.h
+++ b/include/trace/events/sunrpc.h
@@ -1790,6 +1790,31 @@ DEFINE_EVENT(svc_rqst_status, svc_send,
TP_PROTO(const struct svc_rqst *rqst, int status),
TP_ARGS(rqst, status));
+TRACE_EVENT(svc_replace_page_err,
+ TP_PROTO(const struct svc_rqst *rqst),
+
+ TP_ARGS(rqst),
+ TP_STRUCT__entry(
+ SVC_RQST_ENDPOINT_FIELDS(rqst)
+
+ __field(const void *, begin)
+ __field(const void *, respages)
+ __field(const void *, nextpage)
+ ),
+
+ TP_fast_assign(
+ SVC_RQST_ENDPOINT_ASSIGNMENTS(rqst);
+
+ __entry->begin = rqst->rq_pages;
+ __entry->respages = rqst->rq_respages;
+ __entry->nextpage = rqst->rq_next_page;
+ ),
+
+ TP_printk(SVC_RQST_ENDPOINT_FORMAT " begin=%p respages=%p nextpage=%p",
+ SVC_RQST_ENDPOINT_VARARGS,
+ __entry->begin, __entry->respages, __entry->nextpage)
+);
+
TRACE_EVENT(svc_stats_latency,
TP_PROTO(
const struct svc_rqst *rqst
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index fea7ce8fba14..633aa1eb476b 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -842,9 +842,21 @@ EXPORT_SYMBOL_GPL(svc_set_num_threads);
*
* When replacing a page in rq_pages, batch the release of the
* replaced pages to avoid hammering the page allocator.
+ *
+ * Return values:
+ * %true: page replaced
+ * %false: array bounds checking failed
*/
-void svc_rqst_replace_page(struct svc_rqst *rqstp, struct page *page)
+bool svc_rqst_replace_page(struct svc_rqst *rqstp, struct page *page)
{
+ struct page **begin = rqstp->rq_pages;
+ struct page **end = &rqstp->rq_pages[RPCSVC_MAXPAGES];
+
+ if (unlikely(rqstp->rq_next_page < begin || rqstp->rq_next_page > end)) {
+ trace_svc_replace_page_err(rqstp);
+ return false;
+ }
+
if (*rqstp->rq_next_page) {
if (!pagevec_space(&rqstp->rq_pvec))
__pagevec_release(&rqstp->rq_pvec);
@@ -853,6 +865,7 @@ void svc_rqst_replace_page(struct svc_rqst *rqstp, struct page *page)
get_page(page);
*(rqstp->rq_next_page++) = page;
+ return true;
}
EXPORT_SYMBOL_GPL(svc_rqst_replace_page);
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v1 3/3] NFSD: Watch for rq_pages bounds checking errors in nfsd_splice_actor()
2023-03-17 23:01 [PATCH v1 0/3] rq_pages bounds checking Chuck Lever
2023-03-17 23:01 ` [PATCH v1 1/3] nfsd: don't replace page in rq_pages if it's a continuation of last page Chuck Lever
2023-03-17 23:01 ` [PATCH v1 2/3] SUNRPC: add bounds checking to svc_rqst_replace_page Chuck Lever
@ 2023-03-17 23:01 ` Chuck Lever
2023-03-18 10:04 ` [PATCH v1 0/3] rq_pages bounds checking Jeff Layton
3 siblings, 0 replies; 7+ messages in thread
From: Chuck Lever @ 2023-03-17 23:01 UTC (permalink / raw)
To: linux-nfs; +Cc: viro, dcritch, d.lesca
From: Chuck Lever <chuck.lever@oracle.com>
This is a "should never happen" condition, but if for some reason
the pipe splice actor should attempt to walk past the end of
rq_pages, it needs to terminate the READ operation to prevent
corruption of the pointer addresses in the fields just beyond the
array.
A server crash is thus prevented. Since the code is not behaving,
the READ operation returns -EIO to the client. None of the READ
payload data can be trusted if the splice actor isn't operating as
expected.
Suggested-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
fs/nfsd/vfs.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 5783209f17fc..10aa68ca82ef 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -930,6 +930,9 @@ nfsd_open_verified(struct svc_rqst *rqstp, struct svc_fh *fhp, int may_flags,
* Grab and keep cached pages associated with a file in the svc_rqst
* so that they can be passed to the network sendmsg/sendpage routines
* directly. They will be released after the sending has completed.
+ *
+ * Return values: Number of bytes consumed, or -EIO if there are no
+ * remaining pages in rqstp->rq_pages.
*/
static int
nfsd_splice_actor(struct pipe_inode_info *pipe, struct pipe_buffer *buf,
@@ -948,7 +951,8 @@ nfsd_splice_actor(struct pipe_inode_info *pipe, struct pipe_buffer *buf,
*/
if (page == *(rqstp->rq_next_page - 1))
continue;
- svc_rqst_replace_page(rqstp, page);
+ if (unlikely(!svc_rqst_replace_page(rqstp, page)))
+ return -EIO;
}
if (rqstp->rq_res.page_len == 0) // first call
rqstp->rq_res.page_base = offset % PAGE_SIZE;
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v1 0/3] rq_pages bounds checking
2023-03-17 23:01 [PATCH v1 0/3] rq_pages bounds checking Chuck Lever
` (2 preceding siblings ...)
2023-03-17 23:01 ` [PATCH v1 3/3] NFSD: Watch for rq_pages bounds checking errors in nfsd_splice_actor() Chuck Lever
@ 2023-03-18 10:04 ` Jeff Layton
2023-03-18 15:20 ` Chuck Lever III
3 siblings, 1 reply; 7+ messages in thread
From: Jeff Layton @ 2023-03-18 10:04 UTC (permalink / raw)
To: Chuck Lever, linux-nfs; +Cc: viro, dcritch, d.lesca
On Fri, 2023-03-17 at 19:01 -0400, Chuck Lever wrote:
> A slightly modified take on Jeff's earlier patches, tested with
> both NFSv3 and NFSv4.1 via simple fault injection in
> svc_rqst_replace_page().
>
> In general I'm in favor of more rq_pages bounds checking by
> replacing direct modification of the rq_respages and rq_next_page
> fields with accessor functions.
>
> ---
>
> Chuck Lever (2):
> SUNRPC: add bounds checking to svc_rqst_replace_page
> NFSD: Watch for rq_pages bounds checking errors in nfsd_splice_actor()
>
> Jeff Layton (1):
> nfsd: don't replace page in rq_pages if it's a continuation of last page
>
>
> fs/nfsd/vfs.c | 15 +++++++++++++--
> include/linux/sunrpc/svc.h | 2 +-
> include/trace/events/sunrpc.h | 25 +++++++++++++++++++++++++
> net/sunrpc/svc.c | 15 ++++++++++++++-
> 4 files changed, 53 insertions(+), 4 deletions(-)
>
> --
> Chuck Lever
>
Looks good, Chuck, thanks. You can add this to the last two:
Reviewed-by: Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 0/3] rq_pages bounds checking
2023-03-18 10:04 ` [PATCH v1 0/3] rq_pages bounds checking Jeff Layton
@ 2023-03-18 15:20 ` Chuck Lever III
2023-03-18 16:37 ` Jeff Layton
0 siblings, 1 reply; 7+ messages in thread
From: Chuck Lever III @ 2023-03-18 15:20 UTC (permalink / raw)
To: Jeff Layton
Cc: Chuck Lever, Linux NFS Mailing List, Al Viro, dcritch, d.lesca
> On Mar 18, 2023, at 6:04 AM, Jeff Layton <jlayton@kernel.org> wrote:
>
> On Fri, 2023-03-17 at 19:01 -0400, Chuck Lever wrote:
>> A slightly modified take on Jeff's earlier patches, tested with
>> both NFSv3 and NFSv4.1 via simple fault injection in
>> svc_rqst_replace_page().
>>
>> In general I'm in favor of more rq_pages bounds checking by
>> replacing direct modification of the rq_respages and rq_next_page
>> fields with accessor functions.
>>
>> ---
>>
>> Chuck Lever (2):
>> SUNRPC: add bounds checking to svc_rqst_replace_page
>> NFSD: Watch for rq_pages bounds checking errors in nfsd_splice_actor()
>>
>> Jeff Layton (1):
>> nfsd: don't replace page in rq_pages if it's a continuation of last page
>>
>>
>> fs/nfsd/vfs.c | 15 +++++++++++++--
>> include/linux/sunrpc/svc.h | 2 +-
>> include/trace/events/sunrpc.h | 25 +++++++++++++++++++++++++
>> net/sunrpc/svc.c | 15 ++++++++++++++-
>> 4 files changed, 53 insertions(+), 4 deletions(-)
>>
>> --
>> Chuck Lever
>>
>
> Looks good, Chuck, thanks. You can add this to the last two:
>
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
Excellent, thanks!
When I started I expected 3/3 to be more substantial, but since it's
just a handful of lines and the patch descriptions are about the same,
I'm going to squash 2/3 and 3/3 together.
Only question is whether to apply that to nfsd-next or nfsd-fixes.
Since it's a defensive change, I was thinking nfsd-next. Let me know
if you think it should get merged sooner.
--
Chuck Lever
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 0/3] rq_pages bounds checking
2023-03-18 15:20 ` Chuck Lever III
@ 2023-03-18 16:37 ` Jeff Layton
0 siblings, 0 replies; 7+ messages in thread
From: Jeff Layton @ 2023-03-18 16:37 UTC (permalink / raw)
To: Chuck Lever III
Cc: Chuck Lever, Linux NFS Mailing List, Al Viro, dcritch, d.lesca
On Sat, 2023-03-18 at 15:20 +0000, Chuck Lever III wrote:
>
> > On Mar 18, 2023, at 6:04 AM, Jeff Layton <jlayton@kernel.org> wrote:
> >
> > On Fri, 2023-03-17 at 19:01 -0400, Chuck Lever wrote:
> > > A slightly modified take on Jeff's earlier patches, tested with
> > > both NFSv3 and NFSv4.1 via simple fault injection in
> > > svc_rqst_replace_page().
> > >
> > > In general I'm in favor of more rq_pages bounds checking by
> > > replacing direct modification of the rq_respages and rq_next_page
> > > fields with accessor functions.
> > >
> > > ---
> > >
> > > Chuck Lever (2):
> > > SUNRPC: add bounds checking to svc_rqst_replace_page
> > > NFSD: Watch for rq_pages bounds checking errors in nfsd_splice_actor()
> > >
> > > Jeff Layton (1):
> > > nfsd: don't replace page in rq_pages if it's a continuation of last page
> > >
> > >
> > > fs/nfsd/vfs.c | 15 +++++++++++++--
> > > include/linux/sunrpc/svc.h | 2 +-
> > > include/trace/events/sunrpc.h | 25 +++++++++++++++++++++++++
> > > net/sunrpc/svc.c | 15 ++++++++++++++-
> > > 4 files changed, 53 insertions(+), 4 deletions(-)
> > >
> > > --
> > > Chuck Lever
> > >
> >
> > Looks good, Chuck, thanks. You can add this to the last two:
> >
> > Reviewed-by: Jeff Layton <jlayton@kernel.org>
>
> Excellent, thanks!
>
> When I started I expected 3/3 to be more substantial, but since it's
> just a handful of lines and the patch descriptions are about the same,
> I'm going to squash 2/3 and 3/3 together.
>
> Only question is whether to apply that to nfsd-next or nfsd-fixes.
> Since it's a defensive change, I was thinking nfsd-next. Let me know
> if you think it should get merged sooner.
>
No, that sounds fine for those. Patch #1 needs to go in ASAP, of course.
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-03-18 16:38 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-17 23:01 [PATCH v1 0/3] rq_pages bounds checking Chuck Lever
2023-03-17 23:01 ` [PATCH v1 1/3] nfsd: don't replace page in rq_pages if it's a continuation of last page Chuck Lever
2023-03-17 23:01 ` [PATCH v1 2/3] SUNRPC: add bounds checking to svc_rqst_replace_page Chuck Lever
2023-03-17 23:01 ` [PATCH v1 3/3] NFSD: Watch for rq_pages bounds checking errors in nfsd_splice_actor() Chuck Lever
2023-03-18 10:04 ` [PATCH v1 0/3] rq_pages bounds checking Jeff Layton
2023-03-18 15:20 ` Chuck Lever III
2023-03-18 16:37 ` Jeff Layton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).