Re: [PATCH v2] NFS: Fix use-after-free in nfs4_init_client()

[Trond Myklebust <trondmy@hammerspace.com>]

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:

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com