Re: [PATCH v2] NFSD: Add array bounds-checking in nfsd_iter_read()

[Mike Snitzer <snitzer@kernel.org>]

On Tue, Sep 16, 2025 at 11:24:19AM -0400, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> The *count parameter does not appear to be explicitly restricted
> to being smaller than rsize, so it might be possible to overrun
> the rq_bvec or rq_pages arrays.
> 
> Rather than overrunning these arrays (damage done!) and then WARNING
> once, let's harden the loop so that it terminates before the end of
> the arrays are reached. This should result in a short read, which is
> OK -- clients recover by sending additional READ requests for the
> remaining unread bytes.
> 
> Reported-by: NeilBrown <neil@brown.name>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>

Looks even better.

Reviewed-by: Mike Snitzer <snitzer@kernel.org>


> ---
>  fs/nfsd/vfs.c |   12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> Changes since v1:
> - Check for overrunning rq_pages as well
> - Move bounds checking to top of the loop
> - Move incrementing to the bottom of the loop
> 
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 714777c221ed..2026431500ec 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1115,18 +1115,20 @@ __be32 nfsd_iter_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  
>  	v = 0;
>  	total = *count;
> -	while (total) {
> +	while (total && v < rqstp->rq_maxpages &&
> +	       rqstp->rq_next_page < rqstp->rq_page_end) {
>  		len = min_t(size_t, total, PAGE_SIZE - base);
> -		bvec_set_page(&rqstp->rq_bvec[v], *(rqstp->rq_next_page++),
> +		bvec_set_page(&rqstp->rq_bvec[v], *rqstp->rq_next_page,
>  			      len, base);
> +
>  		total -= len;
> +		++rqstp->rq_next_page;
>  		++v;
>  		base = 0;
>  	}
> -	WARN_ON_ONCE(v > rqstp->rq_maxpages);
>  
> -	trace_nfsd_read_vector(rqstp, fhp, offset, *count);
> -	iov_iter_bvec(&iter, ITER_DEST, rqstp->rq_bvec, v, *count);
> +	trace_nfsd_read_vector(rqstp, fhp, offset, *count - total);
> +	iov_iter_bvec(&iter, ITER_DEST, rqstp->rq_bvec, v, *count - total);
>  	host_err = vfs_iocb_iter_read(file, &kiocb, &iter);
>  	return nfsd_finish_read(rqstp, fhp, file, offset, count, eof, host_err);
>  }
> 
> 
>