Re: [PATCH] nfsd: fix out-of-bounds read of sv_permsocks when starting a thread

[NeilBrown]

On Sun, 18 Oct 2026, Jeff Layton wrote:
> On Sun, 2026-10-18 at 17:03 +0530, Deepanshu Kartikey wrote:
> > nfsd() derives its struct net from the first entry of
> > serv->sv_permsocks using list_entry() without checking that the list
> > is non-empty. If the list is empty, list_entry() returns a bogus
> > svc_xprt computed from the list head embedded in struct svc_serv, and
> > the following perm_sock->xpt_net load reads far past the end of the
> > svc_serv allocation:
> > 
> >   BUG: KASAN: slab-out-of-bounds in nfsd+0x118/0xbc0 fs/nfsd/nfssvc.c:894
> >   Read of size 8 at addr ffff88802d423f50 by task nfsd/5988
> > 
> > The list can be empty because nothing guarantees that a permanent
> > listener exists while a thread starts. With dynamic thread spawning, a
> > thread can begin running after the last listener has been closed and
> > removed.
> > 
> > Take sv_lock, which protects sv_permsocks, and check for an empty list
> > and read xpt_net in the same critical section. If there is no
> > permanent socket, report -ENOTCONN through svc_thread_init_status(),
> > which wakes the creator and exits the thread, instead of dereferencing
> > a bogus pointer.
> > 
> > Fixes: 88c476661719 ("nfsd: pass proper net to nfsd_destroy() from NFSd kthreads")
> > Reported-by: syzbot+561b859d032c8bf2fb53@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Closes: https://syzkaller.appspot.com/bug?extid=561b859d032c8bf2fb53
> > Tested-by: syzbot+561b859d032c8bf2fb53@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Signed-off-by: Deepanshu Kartikey <kartikey406@xxxxxxxxx>
> > ---
> >  fs/nfsd/nfssvc.c | 22 ++++++++++++++++++----
> >  1 file changed, 18 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> > index 2edf716ea022..da6fce79efae 100644
> > --- a/fs/nfsd/nfssvc.c
> > +++ b/fs/nfsd/nfssvc.c
> > @@ -890,17 +890,31 @@ nfsd(void *vrqstp)
> >  {
> >  	struct svc_rqst *rqstp = (struct svc_rqst *) vrqstp;
> >  	struct svc_pool *pool = rqstp->rq_pool;
> > -	struct svc_xprt *perm_sock = list_entry(rqstp->rq_server->sv_permsocks.next, typeof(struct svc_xprt), xpt_list);
> > -	struct net *net = perm_sock->xpt_net;
> > -	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> > +	struct svc_serv *serv = rqstp->rq_server;
> > +	struct svc_xprt *perm_sock;
> > +	struct net *net = NULL;
> > +	struct nfsd_net *nn = NULL;
> >  	struct nfsd_thread_local_info ntli = { };
> >  	bool have_mutex = false;
> > +	int err = 0;
> > +
> > +	spin_lock_bh(&serv->sv_lock);
> > +	if (list_empty(&serv->sv_permsocks)) {
> > +		err = -ENOTCONN;
> > +	} else {
> > +		perm_sock = list_first_entry(&serv->sv_permsocks,
> > +					     struct svc_xprt, xpt_list);
> > +		net = perm_sock->xpt_net;
> > +	}
> > +	spin_unlock_bh(&serv->sv_lock);
> > 
> > 
> 
> There is another way to get to the nfsd_net here. It turns out that the
> sv_stats field for nfsd always points to the one embedded in nfsd_net,
> so we could avoid the socket lists altogether by just doing something
> like:
> 
>   struct nfsd_net *nn = container_of(rqstp->rq_server->sv_stats,
>                                    struct nfsd_net, nfsd_svcstats);
> 
> That avoids the extra locking, but it seems a little weird to depend on
> the sv_stats field. Maybe we could wrap that in a little helper or
> something to make it less ugly?
> 
> An alternative would be to add a serv->sv_net field, but most services
> are not per-net. nfsd is the exception so it's the only one that would
> use it.

Most services don't use svc_create_pooled() either - only nfsd.
I would add a 'struct net*' arg to svc_create_pooled() and
store it in serv->sv_net as you suggest.

Thanks,
NeilBrown

> 
> >  
> > +	if (!err)
> > +		nn = net_generic(net, nfsd_net_id);
> >  	/* At this point, the thread shares current->fs
> >  	 * with the init process. We need to create files with the
> >  	 * umask as defined by the client instead of init's umask.
> >  	 */
> > -	svc_thread_init_status(rqstp, unshare_fs_struct());
> > +	svc_thread_init_status(rqstp, err ?: unshare_fs_struct());
> >  
> >  	current->fs->umask = 0;
> >  
> 
> -- 
> Jeff Layton <jlayton@xxxxxxxxxx>
> 
>