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

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

[Chuck Lever]

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>
---
 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);
 }

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

[Mike Snitzer]

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);
>  }
> 
> 
> 

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

[Chuck Lever]

On 9/16/25 9:01 AM, Mike Snitzer wrote:
> 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.

If Neil agrees, I will fold a similar change into the direct read
patch.


> 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);
>>  }
>>
>>
>>


-- 
Chuck Lever

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

[NeilBrown]

On Wed, 17 Sep 2025, 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>
> ---
>  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);
>  }

Very nice - thanks.

Reviewed-by: NeilBrown <neil@brown.name>

NeilBrown

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

[Jeff Layton]

On Tue, 2025-09-16 at 11:24 -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>
> ---
>  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);
>  }
> 
Much nicer.

Reviewed-by: Jeff Layton <jlayton@kernel.org>