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

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

[schumaker.anna]

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);
 	return old;
 
 error:
-- 
2.29.2

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

[Trond Myklebust]

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

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

[Chuck Lever III]

> 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