Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <56b0cb4f-dfe9-6892-7fef-1a2965cf1d99@oracle.com>
Date:   Mon, 12 Dec 2022 05:34:25 -0800
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.5.1
Subject: Re: [PATCH 1/1] NFSD: fix use-after-free in __nfs42_ssc_open()
To:     Jeff Layton <jlayton@kernel.org>, chuck.lever@oracle.com
Cc:     kolga@netapp.com, hdthky0@gmail.com, linux-nfs@vger.kernel.org,
        security@kernel.org
References: <1670786549-27041-1-git-send-email-dai.ngo@oracle.com>
 <7f68cb23445820b4a1c12b74dce0954f537ae5e2.camel@kernel.org>
Content-Language: en-US
From:   dai.ngo@oracle.com
In-Reply-To: <7f68cb23445820b4a1c12b74dce0954f537ae5e2.camel@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?ajV5MVNEb3B2anZjLy9UeW9ydVZmRjduQU5zZUNCcUlRMzFnWi80VHljMEJp?=
 =?utf-8?B?YUtBL1liaUJ1NHl6YUNrSW9qS213eEdCQmRtR1dHRHhFY0JXV2tRWFJ4VDRL?=
 =?utf-8?B?QWxqOTFTWWQvelpiSXFlWEp2OGhKQ0hvRHo0cFJ6UjlCdVFtS0ZoelpKSjZx?=
 =?utf-8?B?dThwWWpQUWJ3SHdOWWxjdjNuaklXeFJMUjNCQzAxc0tpTmtkMDYrWFp0cGFP?=
 =?utf-8?B?WkhvbnU4NnZBVml1ZVgvR0FoWVlMR0x6YTlncUNqcDhPNUlQdGtyLzhFQTlR?=
 =?utf-8?B?YUNkMEpaWkd4TjNmd2FjeUtZa3BWdkMzeExwNC9HRkNRUjFHa2YveGpxQ0Rl?=
 =?utf-8?B?THoxcmVoV0dxZ1c4Zk5FQWVHOC9FaUxCall6OGMyOXN3dkZXZzA1NForK2Jl?=
 =?utf-8?B?dFpTTUozY2hvOXF5YkM5Wi9aRzRaYThNdWU5ZjFMOFNlT1dsMlYrRUQ2Ti9W?=
 =?utf-8?B?ZnQ5azE1OHEvMHZMWVUzZmlNd0x4OG4wd1VFa3ZBSElrcUtJZ1R5cFFpRzVU?=
 =?utf-8?B?a0VmZWc1WXlLTk0vRVI3SlR4dW16MGFjWU1UQnpqNVU4a2h5WmNqbXNsTlVa?=
 =?utf-8?B?RkVYRkhYNjRZRTNXbHluM2pkR0NZZnA5MzVCOXNOKzExSTQrc2lHVkp1MUdh?=
 =?utf-8?B?dlZXNVpjZlVBM0ZhZmV3MmNRajltRWdTOWg2d3BMdzB2VnJwMEI5azZxaXFs?=
 =?utf-8?B?dEw2Yy9wdXJEOXBWSyttemt1UXNnRlFHTStFM1NIc0ZpOHpoOXE3YnN6RnB6?=
 =?utf-8?B?L0M5cnlnOWk4R0g3eStCdFNMSjVWOWN6WEtMMW1jMTBMdE9lZnYralFORWVZ?=
 =?utf-8?B?MEN0U3BmWW53QmZmQTFWVnFSYTRMRExWQytmZFp6aFR4ZXQ3ajRaR0FsR08r?=
 =?utf-8?B?SGtOZzdqY2ZvZWdmeGpvRkFzaklQOTNLeGtPZjcrcEhITHlBd1Q2MENWdmNa?=
 =?utf-8?B?R2RwN0xjTllXcUNQOEVSM3VwQ2gxK0pLbHM4bVNGUU5CU3UxYWpoR2VVSkxO?=
 =?utf-8?B?byswWkhGNzIvb2RGOGhtODhrc2JiaWhMcjE0RVhBL1BTUkpTQndhd2JtdkxG?=
 =?utf-8?B?aGUvYUxoTTAwZmdhcktuWCtJT1dzckpCNGhiUGJ3S3Y4YnM2aGloejRIMkNm?=
 =?utf-8?B?UGUwb0NMUy9UZDg5dENMZjNHWkZTcVpSQUJvdDllK0ZXakZWODRrVXR0dytp?=
 =?utf-8?B?OTYrMGRHUE8zK05pVWo4SnozVXcxWlR0YkN1R2ZPT3JwZGpZQ3JWRkpFbmtl?=
 =?utf-8?B?SHRvUEw4VVdNT1NDNEplNk90bWJ2Y3p4YlFUZFlKVHpGUjN4N21SUlZzUUlv?=
 =?utf-8?B?bUZRMnBNN0daWkxHZzZWM1pUK0RMSHZSam4wVWRjQzBWbW1LK0d6am5lOUZv?=
 =?utf-8?B?R3lpL21iYTMxMTk1eWo1STFmVGZxRmhuR1JOQ1FNazJTcEdYbVc3dERGaWlU?=
 =?utf-8?B?REt1U0hZN0FBVVdHMk1IRDEvY2dwSjV6MHBSUlJuZEpQY0dqcFJjcklOdjNC?=
 =?utf-8?B?Y2p2cUovR0JNem1hbGlITlpyeEdrME8rSHhkTktBV2FoellvNC9sbWp0a2Ir?=
 =?utf-8?B?U2sxYjRTeVBPZEt5RmYrczNYdVFuT2RtbENrOUdkQ2dpMnM2VHFSMFpGSFY0?=
 =?utf-8?B?YzZjMU9uM05udmxNMllEMEZGRmRYQllrdk1rb0JTNDJYNC9CSmd5R2lyelY2?=
 =?utf-8?B?V1dDMUdLU3BsZ1p3OWRPQzVuczF5T2k1R3J1SHZYbCtYYTAxeTAvdGw3bmV0?=
 =?utf-8?B?ZkZ2WWFqaFNmbFgvUlBtVksvSnlvM2RzdUpLQTRqODhzbldUUGdOY053NFJu?=
 =?utf-8?B?RmFZRVdvUDhXYnpiMEFuQUVEMzNUMVQ3UWtpMEkrS3JGUHF2azc0dmQvRjJC?=
 =?utf-8?B?NFRwY0h5ZFFWb2xvbTlTdCtKbjV5ci9HWWlHY3J5WGczR1AxL2lzQkJCUTNS?=
 =?utf-8?B?dWx6T0RHNCtpbkJFRnY4TnhPMzhMOWo4ZVovWStYRzI3Tm1SYkE0ZVlKZkRY?=
 =?utf-8?B?a1BjUVBYRlhkbkdLU3ZFRjhCaGF1TkdEcnVIeTFRU050OVN3NVBrazBjZFRk?=
 =?utf-8?B?S1BoRE15ZC9uYml1ZUlsK2lqOStqZVlaRkdqWkNDV3Bkd2N5VDhvT2JTczNI?=
 =?utf-8?Q?00kMQIZ6/Ouz+TvI8FqgoaXne?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: bc2e699f-6a3b-4534-8dc7-08dadc459751
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4257.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Dec 2022 13:34:27.7623
 (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: yG+f9g2iI7nPbUdd7GBwxN1EFErI5WOWSJ4hXcSFauAlmVTiJRRuWpoVhhJM/fghoT0qFmvJDMx7VyKZnsWuAQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB5648
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.923,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2022-12-12_02,2022-12-12_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 suspectscore=0 mlxscore=0
 mlxlogscore=999 malwarescore=0 spamscore=0 phishscore=0 bulkscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2210170000
 definitions=main-2212120125
X-Proofpoint-ORIG-GUID: Z1Ysvhp55JxE59o1ytYMHMOu9aAIbYZR
X-Proofpoint-GUID: Z1Ysvhp55JxE59o1ytYMHMOu9aAIbYZR
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


On 12/12/22 4:22 AM, Jeff Layton wrote:
> On Sun, 2022-12-11 at 11:22 -0800, Dai Ngo wrote:
>> Problem caused by source's vfsmount being unmounted but remains
>> on the delayed unmount list. This happens when nfs42_ssc_open()
>> return errors.
>> Fixed by removing nfsd4_interssc_connect(), leave the vfsmount
>> for the laundromat to unmount when idle time expires.
>>
>> Reported-by: Xingyuan Mo <hdthky0@gmail.com>
>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>> ---
>>   fs/nfsd/nfs4proc.c | 23 +++++++----------------
>>   1 file changed, 7 insertions(+), 16 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>> index 8beb2bc4c328..756e42cf0d01 100644
>> --- a/fs/nfsd/nfs4proc.c
>> +++ b/fs/nfsd/nfs4proc.c
>> @@ -1463,13 +1463,6 @@ nfsd4_interssc_connect(struct nl4_server *nss, struct svc_rqst *rqstp,
>>   	return status;
>>   }
>>   
>> -static void
>> -nfsd4_interssc_disconnect(struct vfsmount *ss_mnt)
>> -{
>> -	nfs_do_sb_deactive(ss_mnt->mnt_sb);
>> -	mntput(ss_mnt);
>> -}
>> -
>>   /*
>>    * Verify COPY destination stateid.
>>    *
>> @@ -1572,11 +1565,6 @@ nfsd4_cleanup_inter_ssc(struct vfsmount *ss_mnt, struct file *filp,
>>   {
>>   }
>>   
>> -static void
>> -nfsd4_interssc_disconnect(struct vfsmount *ss_mnt)
>> -{
>> -}
>> -
>>   static struct file *nfs42_ssc_open(struct vfsmount *ss_mnt,
>>   				   struct nfs_fh *src_fh,
>>   				   nfs4_stateid *stateid)
>> @@ -1762,7 +1750,8 @@ static int nfsd4_do_async_copy(void *data)
>>   		struct file *filp;
>>   
>>   		filp = nfs42_ssc_open(copy->ss_mnt, &copy->c_fh,
>> -				      &copy->stateid);
>> +					&copy->stateid);
>> +
>>   		if (IS_ERR(filp)) {
>>   			switch (PTR_ERR(filp)) {
>>   			case -EBADF:
>> @@ -1771,7 +1760,7 @@ static int nfsd4_do_async_copy(void *data)
>>   			default:
>>   				nfserr = nfserr_offload_denied;
>>   			}
>> -			nfsd4_interssc_disconnect(copy->ss_mnt);
>> +			/* ss_mnt will be unmounted by the laundromat */
>>   			goto do_callback;
>>   		}
>>   		nfserr = nfsd4_do_copy(copy, filp, copy->nf_dst->nf_file,
>> @@ -1852,8 +1841,10 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>   	if (async_copy)
>>   		cleanup_async_copy(async_copy);
>>   	status = nfserrno(-ENOMEM);
>> -	if (nfsd4_ssc_is_inter(copy))
>> -		nfsd4_interssc_disconnect(copy->ss_mnt);
>> +	/*
>> +	 * source's vfsmount of inter-copy will be unmounted
>> +	 * by the laundromat
>> +	 */
>>   	goto out;
>>   }
>>   
> This looks reasonable at first glance, but I have some concerns with the
> refcounting around ss_mnt elsewhere in this code. nfsd4_ssc_setup_dul
> looks for an existing connection and bumps the ni->nsui_refcnt if it
> finds one.
>
> But then later, nfsd4_cleanup_inter_ssc has a couple of cases where it
> just does a bare mntput:
>
>          if (!nn) {
>                  mntput(ss_mnt);
>                  return;
>          }
> ...
>          if (!found) {
>                  mntput(ss_mnt);
>                  return;
>          }
>
> The first one looks bogus. Can net_generic return NULL? If so how, and
> why is it not a problem elsewhere in the kernel?

it looks like net_generic can not fail, no where else check for NULL
so I will remove this check.

>
> For the second case, if the ni is no longer on the list, where did the
> extra ss_mnt reference come from? Maybe that should be a WARN_ON or
> BUG_ON?

if ni is not found on the list then it's a bug somewhere so I will add
a BUG_ON on this.

Thanks,
-Dai