Re: [PATCH net-next] fs: nfsd: Fix buffer overflow in write_pool_threads()

[David Laight]

On Sun, 16 Aug 2026 13:43:58 -0400
"Chuck Lever" <cel@xxxxxxxxxx> wrote:

> On Wed, Aug 12, 2026, at 5:04 PM, David Laight wrote:
> > On Wed, 12 Aug 2026 16:19:22 -0400
> > "Chuck Lever" <cel@xxxxxxxxxx> wrote:
> >  
> >> On Wed, Aug 12, 2026, at 3:33 PM, David Laight wrote:  
> >> > write_pool_threads() writes the number of threads in each pool into a
> >> > caller-supplied 'almost PAGE_SIZE' buffer.
> >> > If there are enough pools to overflow the buffer the code continues
> >> > writing beynd its end.
> >> >
> >> > Fix the overflow check so that it actually works.
> >> >
> >> > Fixes: eed2965af1bae "knfsd: allow admin to set nthreads per node"
> >> > Signed-off-by: David Laight <david.laight.linux@xxxxxxxxx>
> >> > ---
> >> >
> >> > I'm pretty sure this is 'root only' code.
> >> > So you'd have to try very hard to actually get the overflow.
> >> >
> >> >  fs/nfsd/nfsctl.c | 10 +++++-----
> >> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >> >
> >> > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> >> > index 39e7012a60d8..b74048aa2402 100644
> >> > --- a/fs/nfsd/nfsctl.c
> >> > +++ b/fs/nfsd/nfsctl.c
> >> > @@ -483,8 +483,7 @@ static ssize_t write_pool_threads(struct file 
> >> > *file, char *buf, size_t size)
> >> >  		 * file, sorry.  Report zero threads.
> >> >  		 */
> >> >  		mutex_unlock(&nfsd_mutex);
> >> > -		strcpy(buf, "0\n");
> >> > -		return strlen(buf);
> >> > +		return strscpy(buf, "0\n", SIMPLE_TRANSACTION_LIMIT);
> >> >  	}
> >> > 
> >> >  	nthreads = kzalloc_objs(int, npools);
> >> > @@ -523,13 +522,14 @@ static ssize_t write_pool_threads(struct file 
> >> > *file, char *buf, size_t size)
> >> > 
> >> >  	mesg = buf;
> >> >  	size = SIMPLE_TRANSACTION_LIMIT;
> >> > -	for (i = 0; i < npools && size > 0; i++) {
> >> > -		snprintf(mesg, size, "%d%c", nthreads[i], (i == npools-1 ? '\n' : ' '));
> >> > -		len = strlen(mesg);
> >> > +	for (i = 0; i < npools; i++) {
> >> > +		len = scnprintf(mesg, size, "%d ", nthreads[i]);
> >> >  		size -= len;
> >> >  		mesg += len;
> >> >  	}
> >> >  	rv = mesg - buf;
> >> > +	if (rv != SIMPLE_TRANSACTION_LIMIT - 1)
> >> > +		msg[-1] = '\n';    
> >> 
> >> Did you mean "mesg[-1] = '\n';" here?  
> >
> > Yes - and I thought I'd compiled it ...
> > I did decide not to worry about the missing '\n' when the output 'just fits'.
> > After all you need over 1300 pools with 10 threads to get to 4k.
> > I'd bet something else dies first.
> >
> > 	David
> >
> > (Oh I've replaced Linus's 20 year old email I copied from the commit with
> > his current one (he acked it) and deleted the broken one from the author.)  
> 
> I've convinced myself that the actual problem is the truncation. The
> buffer overflow cannot occur.

My analysis was:
size is size_t so unsigned.
So after snprintf() truncates the 'size -= len' is going to generate a
big number, so the 'size > 0' check doesn't terminate the loop.
Which would mean that the next iteration would write beyond the buffer.
But I think there is a check in snprintf() for 'silly' lengths that may
stop the overwrite.
That doesn't stop the length returned to the caller being longer that
the buffer.
I didn't look what happens as the call site, the buffer will be copied
back to user, if the user provided a long enough buffer that might read
beyond the end of the page.

Of course this is all likely impossible for other reasons.
(and root only).

	David

> I've posted a patch to address that.
> 
>