Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <dd2900e4-fbfc-67c0-73cd-9ddcb1963737@oracle.com>
Date:   Wed, 21 Sep 2022 12:31:58 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.13.0
Subject: Re: [PATCH] mm: hugetlb: fix UAF in hugetlb_handle_userfault
Content-Language: en-US
To:     Liu Shixin <liushixin2@huawei.com>,
        Liu Zixian <liuzixian4@huawei.com>,
        Mike Kravetz <mike.kravetz@oracle.com>,
        Muchun Song <songmuchun@bytedance.com>,
        Andrew Morton <akpm@linux-foundation.org>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Kefeng Wang <wangkefeng.wang@huawei.com>
References: <20220921083440.1267903-1-liushixin2@huawei.com>
From:   Sidhartha Kumar <sidhartha.kumar@oracle.com>
In-Reply-To: <20220921083440.1267903-1-liushixin2@huawei.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?R2JIR1RxQ2tjeWsvbS9uZnlsL09weFlsd3NPREN0VEZSOWppMGdsRlZQV2M1?=
 =?utf-8?B?LzdFWkpNZDAzYVZPTE1rdEtVYnAwZTJLdWo3SHN2a0hMdm9MWlV4eGkrOVpW?=
 =?utf-8?B?M2xaNjM3QUVFbW9KTklDOXRkZXBXSFJ5aWRXUG00dnA4WFJhMlFFUHJWVmNs?=
 =?utf-8?B?dHBTbVBRVWsrMGVad3paODVTTEZSemppWjdQZ3BXOFAwakZvdHR5c0p4M1By?=
 =?utf-8?B?ZFh4QmxKVjVRV3Rzd21RaHcxbTN6R2dUMWh2SGxTZDNGdGFmc2RHRmpjb2hm?=
 =?utf-8?B?QUJRZlpmVlBzVTlZb2NSN3hzMGNXWTd3bmdualU3T2RwN3ppSkp5NStxR3FJ?=
 =?utf-8?B?bFFwTk5zYnRzQmVjSWxFTEQ4Q29iWVM0Ukx0eEJMNXdTdjNweWIzOFV0Wk4x?=
 =?utf-8?B?ZnYzaStDZG02Mk5LNW9uSVZnbXZKL0toTWNtdzgyNGtnbEhMclZHTjlQblls?=
 =?utf-8?B?d3dEdjJuOUt6RVphVm9QZnlqWlJ3UTF5anhUdU9HZzV5ZUpRbys3UXhLVGZs?=
 =?utf-8?B?ZjZNSmVUbmc3OUk2YU1ib1I3T1R0L1R3L1FEeVdLeWY4V0U1QTdkNEp0dnNF?=
 =?utf-8?B?SGlJVWliM1ZZOHV2UE1jaGJyY2c5ZVpvK3lJTnFiT3JCVWEyTFBRZEI5a0Jo?=
 =?utf-8?B?ekpOaEpvTU0xaHlVWlNWR0RtRS83TFMyVzhyaTloN3JPV3ZRYU0xYUlWUGtB?=
 =?utf-8?B?ZUwycHBSaFFsc2dNK0Q2QWtSSGsxaWJiVHZKbEFQNG1wYS82dTJwaDh2dXBr?=
 =?utf-8?B?Y2VrMGV3WjJXK0pFVUNSaERsQ1Y4R1BEa01JeHhDK0k3cTRoUHVkZW5Vb1NN?=
 =?utf-8?B?cWVqUnpCVVkySUdINVRuVGhWcnZyWTRDUndEZFVadUVPU0JtZVlUUjVtZUlm?=
 =?utf-8?B?SHF2SVRoZFNYd1JFNE0wRlNQM0V0aFVSbHVXNTJxdXFPWnl6ZVFDSS82eHRH?=
 =?utf-8?B?ak1zWnlCWHpKMmlzUGdTY0dwTkNCcDZHV1JGV2IrMTRZaU8yZUNJTG5LQWhu?=
 =?utf-8?B?MDMxellDcm82d3FoNFNGamRud3ZxR1p3SzBmZVZzdUlOUjZCWldudFBueGNY?=
 =?utf-8?B?enA5ODJZd2tDZU9uSitEejZETVZ4ZmZ2RStvKzlCUVphQXdyMmNYblNZNzRN?=
 =?utf-8?B?ZngrMStIeDYxZHAwZ2g5USthQ1Q2Z1d1dFJJVTNKanpLSitsbjhiTjQxYmc1?=
 =?utf-8?B?TnBTemNFQThQUGxLL0tpZS9sZkwvNlhORVFBc3BCTVFwZ3JXSlJRR1pNeEVp?=
 =?utf-8?B?UFE3bFRMb0lhZFF6WHVpWmpnQnJURjdPRXI5aGJ0SERTT1RGOTh6UXl6aC9P?=
 =?utf-8?B?MjUrcmYvWDhHR3dZZndSUnE0UVJOQnVvS1hmT1d5ZWh6OW16bTZNYkNLWDBo?=
 =?utf-8?B?enRIM25yb3F2dm1mdVhOc3ZCbG5Ud1pROFA3UDVwcFNpaTNmN21BamdlUjBj?=
 =?utf-8?B?Rnl1clIrYy95RFh0cGlHQXJNR2FldUFTaDBuUFBtSlJPT1l1TXFUWlFEdHJI?=
 =?utf-8?B?dkdtRm00cEw4dUZ4UWxWS3E0NGxYN0ZMbzlZQXZ2UTZiZW5tMU1Od0IzeEFR?=
 =?utf-8?B?eTlPT0dsd0VTNzVVYUc5c21xK0M2QjZaOVBuSXUrWXkrclY0WGFHNzM5Mk9n?=
 =?utf-8?B?a3FFZnpNY0QxZnFQdGNPdVI2UWZib1pza29MWWphNWpXQ3JhcVp1b2RTdy91?=
 =?utf-8?B?M3FjRFpkaU12MHNiODlBenVveXQrU1F6cnBvYjVQdHhRMG56aENhZDU1N2hU?=
 =?utf-8?B?SjZpRklwRE9DU3ltbUE4cTlzWjg3ZDNtMlpVdW12R3VRaXhySzBoaGxFYVUw?=
 =?utf-8?B?UUlnV3haZlE4TTl1NjRrZGNpSVphd2VCV0RBN1hORTZxWnRBZmE0OWg4MFFF?=
 =?utf-8?B?UGpyV2MyL1VaaFljNnkyenZLV3V4Y2thcU05dU1SZG1zV1lYUUpaanMvSmFW?=
 =?utf-8?B?WTYvUFNGRHkvbVh6aGczS3BTdU1sREFEQ1VlUDJYbERHL0F1TlNLOExOR3NX?=
 =?utf-8?B?SXE1czRzZ21mM051ZExNbDlwaURYdGtUR0Fnc3lEaVRWb01LSStsWm94SUQ3?=
 =?utf-8?B?K1NRQW9ZVzhVdVVUMGlCaXFrN1lhclB5YWYzQ3ZMRjBLTWdwZnhrS2lwTEJS?=
 =?utf-8?B?RzNlTDY4bkNITVBMSDlDd3U5OXU1NWxBRGQweFB0a2Z1NGV3b0lKa0VCNlVC?=
 =?utf-8?Q?pQb2GYB5I2vVp/wO/Iq7WLQ=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 092967b7-c8ef-4f45-0e3e-08da9bf73299
X-MS-Exchange-CrossTenant-AuthSource: CH0PR10MB5113.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Sep 2022 17:32:03.8527
 (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: UZMsEZeND+Y7dvYjUy37FL5O4MtGV+6b2XS4K+XSpR7L2SY6p4VMk3qXXVXgiKJ2LwAlxA7A2S8lVd/kYBLywBHLSS0qVVVORZrF8FTzwlM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BLAPR10MB4979
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.528,FMLib:17.11.122.1
 definitions=2022-09-21_09,2022-09-20_02,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 mlxlogscore=999
 spamscore=0 adultscore=0 mlxscore=0 malwarescore=0 bulkscore=0
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2209130000 definitions=main-2209210118
X-Proofpoint-ORIG-GUID: GJrh11ICpAUMALiqhzp83TOQVMhS91sq
X-Proofpoint-GUID: GJrh11ICpAUMALiqhzp83TOQVMhS91sq
X-Spam-Status: No, score=-6.5 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-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 9/21/22 3:34 AM, Liu Shixin wrote:
> The vma_lock and hugetlb_fault_mutex are dropped before handling
> userfault and reacquire them again after handle_userfault(), but
> reacquire the vma_lock could lead to UAF[1] due to the following
> race,
>
> hugetlb_fault
>    hugetlb_no_page
>      /*unlock vma_lock */
>      hugetlb_handle_userfault
>        handle_userfault
>          /* unlock mm->mmap_lock*/
>                                             vm_mmap_pgoff
>                                               do_mmap
>                                                 mmap_region
>                                                   munmap_vma_range
>                                                     /* clean old vma */
>          /* lock vma_lock again  <--- UAF */
>      /* unlock vma_lock */
>
> Since the vma_lock will unlock immediately after hugetlb_handle_userfault(),
> let's drop the unneeded lock and unlock in hugetlb_handle_userfault() to fix
> the issue.
>
> [1] https://lore.kernel.org/linux-mm/20220921014457.1668-1-liuzixian4@huawei.com/
> Reported-by: Liu Zixian <liuzixian4@huawei.com>
> Signed-off-by: Liu Shixin <liushixin2@huawei.com>
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
>   mm/hugetlb.c | 30 +++++++++++-------------------
>   1 file changed, 11 insertions(+), 19 deletions(-)
>
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 9b8526d27c29..5a5d466692cf 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -5489,7 +5489,6 @@ static inline vm_fault_t hugetlb_handle_userfault(struct vm_area_struct *vma,
>   						  unsigned long addr,
>   						  unsigned long reason)
>   {
> -	vm_fault_t ret;
>   	u32 hash;
>   	struct vm_fault vmf = {
>   		.vma = vma,
> @@ -5508,17 +5507,12 @@ static inline vm_fault_t hugetlb_handle_userfault(struct vm_area_struct *vma,
>   
>   	/*
>   	 * vma_lock and hugetlb_fault_mutex must be
> -	 * dropped before handling userfault.  Reacquire
> -	 * after handling fault to make calling code simpler.
> +	 * dropped before handling userfault.
>   	 */
>   	hugetlb_vma_unlock_read(vma);
>   	hash = hugetlb_fault_mutex_hash(mapping, idx);
>   	mutex_unlock(&hugetlb_fault_mutex_table[hash]);
> -	ret = handle_userfault(&vmf, reason);
> -	mutex_lock(&hugetlb_fault_mutex_table[hash]);
> -	hugetlb_vma_lock_read(vma);
> -
> -	return ret;
> +	return handle_userfault(&vmf, reason);
>   }
>   
>   static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
> @@ -5537,6 +5531,7 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
>   	unsigned long haddr = address & huge_page_mask(h);
>   	bool new_page, new_pagecache_page = false;
>   	bool reserve_alloc = false;
> +	u32 hash = hugetlb_fault_mutex_hash(mapping, idx);
>   
>   	/*
>   	 * Currently, we are forced to kill the process in the event the
> @@ -5547,7 +5542,7 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
>   	if (is_vma_resv_set(vma, HPAGE_RESV_UNMAPPED)) {
>   		pr_warn_ratelimited("PID %d killed due to inadequate hugepage pool\n",
>   			   current->pid);
> -		return ret;
> +		goto out;
>   	}
>   
>   	/*
> @@ -5561,12 +5556,10 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
>   		if (idx >= size)
>   			goto out;
>   		/* Check for page in userfault range */
> -		if (userfaultfd_missing(vma)) {
> -			ret = hugetlb_handle_userfault(vma, mapping, idx,
> +		if (userfaultfd_missing(vma))
> +			return hugetlb_handle_userfault(vma, mapping, idx,
>   						       flags, haddr, address,
>   						       VM_UFFD_MISSING);
> -			goto out;
> -		}
>   
>   		page = alloc_huge_page(vma, haddr, 0);
>   		if (IS_ERR(page)) {
> @@ -5634,10 +5627,9 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
>   		if (userfaultfd_minor(vma)) {
>   			unlock_page(page);
>   			put_page(page);
> -			ret = hugetlb_handle_userfault(vma, mapping, idx,
> +			return hugetlb_handle_userfault(vma, mapping, idx,
>   						       flags, haddr, address,
>   						       VM_UFFD_MINOR);
> -			goto out;
>   		}
>   	}
>   
> @@ -5695,6 +5687,8 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
>   
>   	unlock_page(page);
>   out:
> +	hugetlb_vma_unlock_read(vma);
> +	mutex_unlock(&hugetlb_fault_mutex_table[hash]);
>   	return ret;
>   
>   backout:
> @@ -5792,11 +5786,9 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
>   
>   	entry = huge_ptep_get(ptep);
>   	/* PTE markers should be handled the same way as none pte */
> -	if (huge_pte_none_mostly(entry)) {
> -		ret = hugetlb_no_page(mm, vma, mapping, idx, address, ptep,
> +	if (huge_pte_none_mostly(entry))
> +		return hugetlb_no_page(mm, vma, mapping, idx, address, ptep,
>   				      entry, flags);
> -		goto out_mutex;
> -	}
>   
>   	ret = 0;
>   

I've been looking at this as well.
Reviewed-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>