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

[Deepanshu Kartikey]

On Mon, Oct 19, 2026 at 2:15 AM NeilBrown <neilb@xxxxxxxxxxx> wrote:
>
> 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>
> >
> >
>


Thanks Jeff and Neil, for the review. I will make the changes and
submit patch v2 shortly.

Thanks

Deepanshu