Re: [PATCH] mm: hugetlb: fix UAF in hugetlb_handle_userfault

[Sidhartha Kumar <sidhartha.kumar@oracle.com>]

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>