| Message ID | 20210602183120.532206-1-Anna.Schumaker@Netapp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] NFS: Fix use-after-free in nfs4_init_client() | expand |
On Wed, 2021-06-02 at 14:31 -0400, schumaker.anna@gmail.com wrote: > From: Anna Schumaker <Anna.Schumaker@Netapp.com> > > KASAN reports a use-after-free when attempting to mount two different > exports through two different NICs that belong to the same server. > > Olga was able to hit this with kernels starting somewhere between 5.7 > and 5.10, but I traced the patch that introduced the clear_bit() call > to > 4.13. So something must have changed in the refcounting of the clp > pointer to make this call to nfs_put_client() the very last one. > > Fixes: 8dcbec6d20 ("NFSv41: Handle EXCHID4_FLAG_CONFIRMED_R during > NFSv4.1 migration") > Cc: stable@vger.kernel.org # 4.13+ > Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com> > --- > v2: No changes except adding the fixes tag that I initially forgot > --- > fs/nfs/nfs4client.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c > index 889a9f4c0310..42719384e25f 100644 > --- a/fs/nfs/nfs4client.c > +++ b/fs/nfs/nfs4client.c > @@ -435,8 +435,8 @@ struct nfs_client *nfs4_init_client(struct > nfs_client *clp, > */ > nfs_mark_client_ready(clp, -EPERM); > } > - nfs_put_client(clp); > clear_bit(NFS_CS_TSM_POSSIBLE, &clp->cl_flags); > + nfs_put_client(clp); OK. I'm reading this, and it is not making sense to me. Why are we changing a flag on an object that is about to be destroyed by the nfs_put_client() anyway? Let's go back to the author of commit 8dcbec6d20 and ask him. Chuck, is it possible that you were actually intending to clear NFS_CS_TSM_POSSIBLE on &old->cl_flags (i.e. on the object that is actually returned by the call to nfs4_init_client())? > return old; > > error:
> On Jun 2, 2021, at 2:52 PM, Trond Myklebust <trondmy@hammerspace.com> wrote: > > On Wed, 2021-06-02 at 14:31 -0400, schumaker.anna@gmail.com wrote: >> From: Anna Schumaker <Anna.Schumaker@Netapp.com> >> >> KASAN reports a use-after-free when attempting to mount two different >> exports through two different NICs that belong to the same server. >> >> Olga was able to hit this with kernels starting somewhere between 5.7 >> and 5.10, but I traced the patch that introduced the clear_bit() call >> to >> 4.13. So something must have changed in the refcounting of the clp >> pointer to make this call to nfs_put_client() the very last one. >> >> Fixes: 8dcbec6d20 ("NFSv41: Handle EXCHID4_FLAG_CONFIRMED_R during >> NFSv4.1 migration") >> Cc: stable@vger.kernel.org # 4.13+ >> Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com> >> --- >> v2: No changes except adding the fixes tag that I initially forgot >> --- >> fs/nfs/nfs4client.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c >> index 889a9f4c0310..42719384e25f 100644 >> --- a/fs/nfs/nfs4client.c >> +++ b/fs/nfs/nfs4client.c >> @@ -435,8 +435,8 @@ struct nfs_client *nfs4_init_client(struct >> nfs_client *clp, >> */ >> nfs_mark_client_ready(clp, -EPERM); >> } >> - nfs_put_client(clp); >> clear_bit(NFS_CS_TSM_POSSIBLE, &clp->cl_flags); >> + nfs_put_client(clp); > > OK. I'm reading this, and it is not making sense to me. Why are we > changing a flag on an object that is about to be destroyed by the > nfs_put_client() anyway? Let's go back to the author of commit > 8dcbec6d20 and ask him. > > Chuck, is it possible that you were actually intending to clear > NFS_CS_TSM_POSSIBLE on &old->cl_flags (i.e. on the object that is > actually returned by the call to nfs4_init_client())? Yes, that's plausible. >> return old; >> >> error: > > -- > Trond Myklebust > Linux NFS client maintainer, Hammerspace > trond.myklebust@hammerspace.com > > -- Chuck Lever
diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c index 889a9f4c0310..42719384e25f 100644 --- a/fs/nfs/nfs4client.c +++ b/fs/nfs/nfs4client.c @@ -435,8 +435,8 @@ struct nfs_client *nfs4_init_client(struct nfs_client *clp, */ nfs_mark_client_ready(clp, -EPERM); } - nfs_put_client(clp); clear_bit(NFS_CS_TSM_POSSIBLE, &clp->cl_flags); + nfs_put_client(clp); return old; error: