Re: [PATCH v2] Prevent rqstp->rq_pages[RPCSVC_MAXPAGES] overrun

["J. Bruce Fields" <bfields@fieldses.org>]

On Wed, Jul 27, 2016 at 08:57:26AM +0900, Seiichi Ikarashi wrote:
> Hi Bruce,
> 
> On 2016-07-27 05:19, J. Bruce Fields wrote:
> > Thanks for the report.
> > 
> > On Tue, Jul 26, 2016 at 11:38:11AM +0900, Seiichi Ikarashi wrote:
> >> If over-"RPCSVC_MAXPAGES" pages are sent from file system through pipe_buffer,
> >> nfsd_splice_actor() corrupts struct svc_rqst and results in kernel panic. It
> >> actually occurred with a parallel distributed file system. It needs boundary
> >> checking.
> > 
> > This check might be useful as defensive programming, but the bug was
> > elsewhere.
> 
> Yah, I think the main factor exists in file system and/or VFS splice sides.
> But I also think that NFS should defend itself against overlimit pages
> because the limit is decided by NFS/SUNRPC, not by file system and others.
> 
> > 
> > In theory this should be prevented by the "maxcount" calculations in
> > nfsd4_encode_read().
> 
> The "maxcount" looks just limiting the read length from the file system.
> Is my understanding correct?

Right.

> 
> And the number of pages provided from the file system is up to the file system.
> The file system can split the read data into an arbitrary number of pages.

Oh, so if we ask the filesystem for 3 bytes it might potentially return
those in 3 separate pages?  Is that at all legal?

> > What version of the kernel did you see this happen on?  What was the
> > client, and what was it doing?  Any other hints on reproducing?
> 
> It was a NFSv3 read access from a client of maybe-Linux-based system
> against the server of RHEL6 system exporting a file system that I cannot
> access its source code. Sorry for lack of info.

OK, understood.

--b.

> 
> 
> Seiichi.
> 
> 
> > 
> > --b.
> > 
> >>
> >> v2: Fix semicolon-missing bug.
> >>
> >> Signed-off-by: Seiichi Ikarashi <s.ikarashi@jp.fujitsu.com>
> >>
> >> ---
> >>  fs/nfsd/vfs.c | 8 ++++++++
> >>  1 file changed, 8 insertions(+)
> >>
> >> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> >> index 6fbd81e..43393f3 100644
> >> --- a/fs/nfsd/vfs.c
> >> +++ b/fs/nfsd/vfs.c
> >> @@ -811,12 +811,20 @@ nfsd_splice_actor(struct pipe_inode_info *pipe, struct pipe_buffer *buf,
> >>  	size = sd->len;
> >>  
> >>  	if (rqstp->rq_res.page_len == 0) {
> >> +		if (rqstp->rq_next_page > &rqstp->rq_pages[RPCSVC_MAXPAGES-1]) {
> >> +			WARN_ON(1);
> >> +			return -ENOMEM;
> >> +		}
> >>  		get_page(page);
> >>  		put_page(*rqstp->rq_next_page);
> >>  		*(rqstp->rq_next_page++) = page;
> >>  		rqstp->rq_res.page_base = buf->offset;
> >>  		rqstp->rq_res.page_len = size;
> >>  	} else if (page != pp[-1]) {
> >> +		if (rqstp->rq_next_page > &rqstp->rq_pages[RPCSVC_MAXPAGES-1]) {
> >> +			WARN_ON(1);
> >> +			return -ENOMEM;
> >> +		}
> >>  		get_page(page);
> >>  		if (*rqstp->rq_next_page)
> >>  			put_page(*rqstp->rq_next_page);
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > .
> >