Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Trond Myklebust <trondmy@hammerspace.com>
CC:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Anna Schumaker <schumaker.anna@gmail.com>,
        Anna Schumaker <Anna.Schumaker@Netapp.com>
Subject: Re: [PATCH v2] NFS: Fix use-after-free in nfs4_init_client()
Thread-Topic: [PATCH v2] NFS: Fix use-after-free in nfs4_init_client()
Thread-Index: AQHXV92kqzRlPk8SFkSO0nQlw+cYsqsBETEAgAAEfoA=
Date:   Wed, 2 Jun 2021 19:08:46 +0000
Message-ID: <F84E7E38-B94A-43E9-9CE2-29D277106BA3@oracle.com>
References: <20210602183120.532206-1-Anna.Schumaker@Netapp.com>
 <ed1b9642f6b26f2174c5fd5b88a629a25225b926.camel@hammerspace.com>
In-Reply-To: <ed1b9642f6b26f2174c5fd5b88a629a25225b926.camel@hammerspace.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <15E6FD53716AFB4E89D3220CCD59365B@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 60df8bad-9539-4cf1-9b41-08d925f9d8b1
X-MS-Exchange-CrossTenant-originalarrivaltime: 02 Jun 2021 19:08:46.1392
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: HDvNglyodVe2lSG4y2sZ8TfjW4fLP5U6+IfoMlNWbj3cuXn5X9SKThAp1U5OvUja4xA8CCFB5izp7kJlWeg3ow==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR10MB3221
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=10003 signatures=668682
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 bulkscore=0 phishscore=0
 spamscore=0 malwarescore=0 mlxscore=0 mlxlogscore=999 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2104190000
 definitions=main-2106020122
X-Proofpoint-ORIG-GUID: aHvrl_0lkIPJLrnyoq9qL6M_PU37bwbw
X-Proofpoint-GUID: aHvrl_0lkIPJLrnyoq9qL6M_PU37bwbw
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Jun 2, 2021, at 2:52 PM, Trond Myklebust <trondmy@hammerspace.com> wro=
te:
>=20
> On Wed, 2021-06-02 at 14:31 -0400, schumaker.anna@gmail.com wrote:
>> From: Anna Schumaker <Anna.Schumaker@Netapp.com>
>>=20
>> KASAN reports a use-after-free when attempting to mount two different
>> exports through two different NICs that belong to the same server.
>>=20
>> 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.
>>=20
>> 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(-)
>>=20
>> 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);
>=20
> 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.
>=20
> 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;
>> =20
>>  error:
>=20
> --=20
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
>=20
>=20

--
Chuck Lever