lkml.org 
[lkml]   [2022]   [Sep]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] mm: hugetlb: fix UAF in hugetlb_handle_userfault
From


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>

\
 
 \ /
  Last update: 2022-09-21 19:33    [W:0.124 / U:0.532 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site