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

[Chuck Lever <cel@kernel.org>]

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