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

[Jeff Layton <jlayton@kernel.org>]

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@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=561b859d032c8bf2fb53
> Tested-by: syzbot+561b859d032c8bf2fb53@syzkaller.appspotmail.com
> Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
> ---
>  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.

>  
> +	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@kernel.org>