| Message ID | 20220921083440.1267903-1-liushixin2@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: hugetlb: fix UAF in hugetlb_handle_userfault | expand |
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>
On 09/21/22 16:34, 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. Thank you very much! When I saw this report, the obvious fix was to do something like what you have done below. That looks fine with a few minor comments. One question I have not yet answered is, "Does this same issue apply to follow_hugetlb_page()?". I believe it does. follow_hugetlb_page calls hugetlb_fault which could result in the fault being processed by userfaultfd. If we experience the race above, then the associated vma could no longer be valid when returning from hugetlb_fault. follow_hugetlb_page and callers have a flag (locked) to deal with dropping mmap lock. However, I am not sure if it is handled correctly WRT userfaultfd. I think this needs to be answered before fixing. And, if the follow_hugetlb_page code needs to be fixed it should be done at the same time. > [1] https://lore.kernel.org/linux-mm/20220921014457.1668-1-liuzixian4@huawei.com/ > Reported-by: Liu Zixian <liuzixian4@huawei.com> Perhaps reported by should be, Reported-by: syzbot+193f9cee8638750b23cf@syzkaller.appspotmail.com https://lore.kernel.org/linux-mm/000000000000d5e00a05e834962e@google.com/ Should also add, Fixes: 1a1aad8a9b7b ("userfaultfd: hugetlbfs: add userfaultfd hugetlb hook") as well as, Cc: <stable@vger.kernel.org> > 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 ... > @@ -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)) We should add a big comment noting that hugetlb_no_page will drop vma lock and hugetl fault mutex. This will make it easier for people reading the code and immediately thinking we are returning without dropping the locks.
On Wed, 21 Sep 2022 16:34:40 +0800 Liu Shixin <liushixin2@huawei.com> 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. > > @@ -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); > } Current code is rather different from this. So if the bug still exists in current code, please verify this and redo the patch appropriately? And hang on to this version to help with the -stable backporting. Thanks.
On 09/21/22 10:48, Mike Kravetz wrote: > On 09/21/22 16:34, 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. > > Thank you very much! > > When I saw this report, the obvious fix was to do something like what you have > done below. That looks fine with a few minor comments. > > One question I have not yet answered is, "Does this same issue apply to > follow_hugetlb_page()?". I believe it does. follow_hugetlb_page calls > hugetlb_fault which could result in the fault being processed by userfaultfd. > If we experience the race above, then the associated vma could no longer be > valid when returning from hugetlb_fault. follow_hugetlb_page and callers > have a flag (locked) to deal with dropping mmap lock. However, I am not sure > if it is handled correctly WRT userfaultfd. I think this needs to be answered > before fixing. And, if the follow_hugetlb_page code needs to be fixed it > should be done at the same time. > To at least verify this code path, I added userfaultfd handling to the gup_test program in kernel selftests. When doing basic gup test on a hugetlb page in a userfaultfd registered range, I hit this warning: [ 6939.867796] FAULT_FLAG_ALLOW_RETRY missing 1 [ 6939.871503] CPU: 2 PID: 5720 Comm: gup_test Not tainted 6.0.0-rc6-next-20220921+ #72 [ 6939.874562] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.15.0-1.fc35 04/01/2014 [ 6939.877707] Call Trace: [ 6939.878745] <TASK> [ 6939.879779] dump_stack_lvl+0x6c/0x9f [ 6939.881199] handle_userfault.cold+0x14/0x1e [ 6939.882830] ? find_held_lock+0x2b/0x80 [ 6939.884370] ? __mutex_unlock_slowpath+0x45/0x280 [ 6939.886145] hugetlb_handle_userfault+0x90/0xf0 [ 6939.887936] hugetlb_fault+0xb7e/0xda0 [ 6939.889409] ? vprintk_emit+0x118/0x3a0 [ 6939.890903] ? _printk+0x58/0x73 [ 6939.892279] follow_hugetlb_page.cold+0x59/0x145 [ 6939.894116] __get_user_pages+0x146/0x750 [ 6939.895580] __gup_longterm_locked+0x3e9/0x680 [ 6939.897023] ? seqcount_lockdep_reader_access.constprop.0+0xa5/0xb0 [ 6939.898939] ? lockdep_hardirqs_on+0x7d/0x100 [ 6939.901243] gup_test_ioctl+0x320/0x6e0 [ 6939.902202] __x64_sys_ioctl+0x87/0xc0 [ 6939.903220] do_syscall_64+0x38/0x90 [ 6939.904233] entry_SYSCALL_64_after_hwframe+0x63/0xcd [ 6939.905423] RIP: 0033:0x7fbb53830f7b This is because userfaultfd is expecting FAULT_FLAG_ALLOW_RETRY which is not set in this path. Adding John, Peter and David on Cc: as they are much more fluent in all the fault and FOLL combinations and might have immediate suggestions. It is going to take me a little while to figure out: 1) How to make sure we get the right flags passed to handle_userfault 2) How to modify follow_hugetlb_page as userfaultfd can certainly drop mmap_lock. So we can not assume vma still exists upon return.
On 9/21/22 16:57, Mike Kravetz wrote: > On 09/21/22 10:48, Mike Kravetz wrote: >> On 09/21/22 16:34, 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. >> >> Thank you very much! >> >> When I saw this report, the obvious fix was to do something like what you have >> done below. That looks fine with a few minor comments. >> >> One question I have not yet answered is, "Does this same issue apply to >> follow_hugetlb_page()?". I believe it does. follow_hugetlb_page calls >> hugetlb_fault which could result in the fault being processed by userfaultfd. >> If we experience the race above, then the associated vma could no longer be >> valid when returning from hugetlb_fault. follow_hugetlb_page and callers >> have a flag (locked) to deal with dropping mmap lock. However, I am not sure >> if it is handled correctly WRT userfaultfd. I think this needs to be answered >> before fixing. And, if the follow_hugetlb_page code needs to be fixed it >> should be done at the same time. >> > > To at least verify this code path, I added userfaultfd handling to the gup_test > program in kernel selftests. When doing basic gup test on a hugetlb page in Just for those of us who are easily confused by userfaultfd cases, can you show what that patch is? It would help me understand this a little faster. Actually I'm expecting that Peter can easily answer this whole thing. :) thanks,
On 2022/9/22 3:07, Andrew Morton wrote: > On Wed, 21 Sep 2022 16:34:40 +0800 Liu Shixin <liushixin2@huawei.com> 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. >> >> @@ -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); >> } > Current code is rather different from this. So if the bug still exists > in current code, please verify this and redo the patch appropriately? > > And hang on to this version to help with the -stable backporting. > > Thanks. > . This patch conflicts with patch series "hugetlb: Use new vma lock for huge pmd sharing synchronization". So I reproduce the problem on next-20220920 and this patch is based on next-20220920 instead of mainline. This problem is existed since v4.11. I will send the stable version later.
On 09/21/22 17:57, John Hubbard wrote: > On 9/21/22 16:57, Mike Kravetz wrote: > > On 09/21/22 10:48, Mike Kravetz wrote: > >> On 09/21/22 16:34, 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. > >> > >> Thank you very much! > >> > >> When I saw this report, the obvious fix was to do something like what you have > >> done below. That looks fine with a few minor comments. > >> > >> One question I have not yet answered is, "Does this same issue apply to > >> follow_hugetlb_page()?". I believe it does. follow_hugetlb_page calls > >> hugetlb_fault which could result in the fault being processed by userfaultfd. > >> If we experience the race above, then the associated vma could no longer be > >> valid when returning from hugetlb_fault. follow_hugetlb_page and callers > >> have a flag (locked) to deal with dropping mmap lock. However, I am not sure > >> if it is handled correctly WRT userfaultfd. I think this needs to be answered > >> before fixing. And, if the follow_hugetlb_page code needs to be fixed it > >> should be done at the same time. > >> > > > > To at least verify this code path, I added userfaultfd handling to the gup_test > > program in kernel selftests. When doing basic gup test on a hugetlb page in > > Just for those of us who are easily confused by userfaultfd cases, can you show > what that patch is? It would help me understand this a little faster. The ugly (just throw code at it to make it work) diff is below. All I did was add the sample code from the userfaultfd man page. With that change in place, I just run 'gup_test -U -z -m 2 -H' > Actually I'm expecting that Peter can easily answer this whole thing. :) diff --git a/tools/testing/selftests/vm/gup_test.c b/tools/testing/selftests/vm/gup_test.c index e43879291dac..491424d0a039 100644 --- a/tools/testing/selftests/vm/gup_test.c +++ b/tools/testing/selftests/vm/gup_test.c @@ -8,8 +8,11 @@ #include <sys/mman.h> #include <sys/stat.h> #include <sys/types.h> +#include <sys/syscall.h> #include <pthread.h> #include <assert.h> +#include <poll.h> +#include <linux/userfaultfd.h> #include <mm/gup_test.h> #include "../kselftest.h" @@ -48,6 +51,75 @@ static char *cmd_to_str(unsigned long cmd) return "Unknown command"; } +static void *fault_handler_thread(void *arg) +{ + static struct uffd_msg msg; /* Data read from userfaultfd */ + static int fault_cnt = 0; /* Number of faults so far handled */ + long uffd; + static char *page = NULL; + struct uffdio_copy uffdio_copy; + ssize_t nread; + size_t page_size = 2 * 1024 * 1024; + + uffd = (long) arg; + + if (page == NULL) { + page = mmap(NULL, page_size, PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); + if (page == MAP_FAILED) + exit(1); + } + + for (;;) { + struct pollfd pollfd; + int nready; + pollfd.fd = uffd; + pollfd.events = POLLIN; + nready = poll(&pollfd, 1, -1); + if (nready == -1) + exit(1); + + printf("\nfault_handler_thread():\n"); + printf(" poll() returns: nready = %d; " + "POLLIN = %d; POLLERR = %d\n", nready, + (pollfd.revents & POLLIN) != 0, + (pollfd.revents & POLLERR) != 0); + + nread = read(uffd, &msg, sizeof(msg)); + if (nread == 0) { + printf("EOF on userfaultfd!\n"); + exit(1); + } + + if (nread == -1) { + printf("nread == -1\n"); + exit(1); + } + + if (msg.event != UFFD_EVENT_PAGEFAULT) { + printf("Unexpected event on userfaultfd\n"); + exit(1); + } + + printf(" UFFD_EVENT_PAGEFAULT event: "); + printf("flags = %llx; ", msg.arg.pagefault.flags); + printf("address = %llx\n", msg.arg.pagefault.address); + + memset(page, 'A' + fault_cnt % 20, page_size); + fault_cnt++; + + uffdio_copy.src = (unsigned long) page; + uffdio_copy.dst = (unsigned long) msg.arg.pagefault.address & + ~(page_size - 1); + uffdio_copy.len = page_size; + uffdio_copy.mode = 0; + uffdio_copy.copy = 0; + if (ioctl(uffd, UFFDIO_COPY, &uffdio_copy) == -1) + exit(1); + + } +} + void *gup_thread(void *data) { struct gup_test gup = *(struct gup_test *)data; @@ -94,7 +166,11 @@ int main(int argc, char **argv) int flags = MAP_PRIVATE, touch = 0; char *file = "/dev/zero"; pthread_t *tid; + pthread_t thr; char *p; + long uffd; + struct uffdio_api uffdio_api; + struct uffdio_register uffdio_register; while ((opt = getopt(argc, argv, "m:r:n:F:f:abcj:tTLUuwWSHpz")) != -1) { switch (opt) { @@ -230,6 +306,18 @@ int main(int argc, char **argv) exit(KSFT_SKIP); } + uffd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK); + if (uffd == -1) { + perror("uffd open"); + exit(1); + } + uffdio_api.api = UFFD_API; + uffdio_api.features = 0; + if (ioctl(uffd, UFFDIO_API, &uffdio_api) == -1) { + perror("uffd ioctl API"); + exit(1); + } + p = mmap(NULL, size, PROT_READ | PROT_WRITE, flags, filed, 0); if (p == MAP_FAILED) { perror("mmap"); @@ -237,6 +325,20 @@ int main(int argc, char **argv) } gup.addr = (unsigned long)p; + uffdio_register.range.start = (unsigned long)p; + uffdio_register.range.len = size; + uffdio_register.mode = UFFDIO_REGISTER_MODE_MISSING; + if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) == -1) { + perror("uffd ioctl API"); + exit(1); + } + ret = pthread_create(&thr, NULL, fault_handler_thread, (void *)uffd); + if (ret) { + exit(1); + } + + sleep(5); + if (thp == 1) madvise(p, size, MADV_HUGEPAGE); else if (thp == 0)
On 22.09.22 01:57, Mike Kravetz wrote: > On 09/21/22 10:48, Mike Kravetz wrote: >> On 09/21/22 16:34, 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. >> >> Thank you very much! >> >> When I saw this report, the obvious fix was to do something like what you have >> done below. That looks fine with a few minor comments. >> >> One question I have not yet answered is, "Does this same issue apply to >> follow_hugetlb_page()?". I believe it does. follow_hugetlb_page calls >> hugetlb_fault which could result in the fault being processed by userfaultfd. >> If we experience the race above, then the associated vma could no longer be >> valid when returning from hugetlb_fault. follow_hugetlb_page and callers >> have a flag (locked) to deal with dropping mmap lock. However, I am not sure >> if it is handled correctly WRT userfaultfd. I think this needs to be answered >> before fixing. And, if the follow_hugetlb_page code needs to be fixed it >> should be done at the same time. >> > > To at least verify this code path, I added userfaultfd handling to the gup_test > program in kernel selftests. When doing basic gup test on a hugetlb page in > a userfaultfd registered range, I hit this warning: > > [ 6939.867796] FAULT_FLAG_ALLOW_RETRY missing 1 > [ 6939.871503] CPU: 2 PID: 5720 Comm: gup_test Not tainted 6.0.0-rc6-next-20220921+ #72 > [ 6939.874562] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.15.0-1.fc35 04/01/2014 > [ 6939.877707] Call Trace: > [ 6939.878745] <TASK> > [ 6939.879779] dump_stack_lvl+0x6c/0x9f > [ 6939.881199] handle_userfault.cold+0x14/0x1e > [ 6939.882830] ? find_held_lock+0x2b/0x80 > [ 6939.884370] ? __mutex_unlock_slowpath+0x45/0x280 > [ 6939.886145] hugetlb_handle_userfault+0x90/0xf0 > [ 6939.887936] hugetlb_fault+0xb7e/0xda0 > [ 6939.889409] ? vprintk_emit+0x118/0x3a0 > [ 6939.890903] ? _printk+0x58/0x73 > [ 6939.892279] follow_hugetlb_page.cold+0x59/0x145 > [ 6939.894116] __get_user_pages+0x146/0x750 > [ 6939.895580] __gup_longterm_locked+0x3e9/0x680 > [ 6939.897023] ? seqcount_lockdep_reader_access.constprop.0+0xa5/0xb0 > [ 6939.898939] ? lockdep_hardirqs_on+0x7d/0x100 > [ 6939.901243] gup_test_ioctl+0x320/0x6e0 > [ 6939.902202] __x64_sys_ioctl+0x87/0xc0 > [ 6939.903220] do_syscall_64+0x38/0x90 > [ 6939.904233] entry_SYSCALL_64_after_hwframe+0x63/0xcd > [ 6939.905423] RIP: 0033:0x7fbb53830f7b > > This is because userfaultfd is expecting FAULT_FLAG_ALLOW_RETRY which is not > set in this path. Right. Without being able to drop the mmap lock, we cannot continue. And we don't know if we can drop it without FAULT_FLAG_ALLOW_RETRY. FAULT_FLAG_ALLOW_RETRY is only set when we can communicate to the caller that we dropped the mmap lock [e.g., int *locked parameter]. All code paths that pass NULL won't be able to handle -- especially surprisingly also pin_user_pages_fast() -- cannot trigger usefaultfd and will result in this warning. A "sane" example is access via /proc/self/mem via ptrace: we don't want to trigger userfaultfd, but instead simply fail the GUP get/pin. Now, this is just a printed *warning* (not a WARN/BUG/taint) that tells us that there is a GUP user that isn't prepared for userfaultfd. So it rather points out a missing GUP adaption -- incomplete userfaultfd support. And we seem to have plenty of that judging that pin_user_pages_fast_only(). Maybe the printed stack trace is a bit too much and makes this look very scary. > > Adding John, Peter and David on Cc: as they are much more fluent in all the > fault and FOLL combinations and might have immediate suggestions. It is going > to take me a little while to figure out: > 1) How to make sure we get the right flags passed to handle_userfault This is a GUP caller problem -- or rather, how GUP has to deal with userfaultfd. > 2) How to modify follow_hugetlb_page as userfaultfd can certainly drop > mmap_lock. So we can not assume vma still exists upon return. Again, we have to communicate to the GUP caller that we dropped the mmap lock. And that requires GUP caller changes.
On Wed, Sep 21, 2022 at 04:57:39PM -0700, Mike Kravetz wrote: > On 09/21/22 10:48, Mike Kravetz wrote: > > On 09/21/22 16:34, 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. > > > > Thank you very much! > > > > When I saw this report, the obvious fix was to do something like what you have > > done below. That looks fine with a few minor comments. > > > > One question I have not yet answered is, "Does this same issue apply to > > follow_hugetlb_page()?". I believe it does. follow_hugetlb_page calls > > hugetlb_fault which could result in the fault being processed by userfaultfd. > > If we experience the race above, then the associated vma could no longer be > > valid when returning from hugetlb_fault. follow_hugetlb_page and callers > > have a flag (locked) to deal with dropping mmap lock. However, I am not sure > > if it is handled correctly WRT userfaultfd. I think this needs to be answered > > before fixing. And, if the follow_hugetlb_page code needs to be fixed it > > should be done at the same time. > > > > To at least verify this code path, I added userfaultfd handling to the gup_test > program in kernel selftests. IIRC vm/userfaultfd should already have GUP tested with pthread mutexes (which iiuc uses futex, and futex uses GUP). But indeed I didn't trigger any GUP paths after a quick run.. I agree we should have some unit test that can at least cover GUP with userfaultfd. I'll further check it up from vm/userfaultfd side later. > When doing basic gup test on a hugetlb page in > a userfaultfd registered range, I hit this warning: > > [ 6939.867796] FAULT_FLAG_ALLOW_RETRY missing 1 > [ 6939.871503] CPU: 2 PID: 5720 Comm: gup_test Not tainted 6.0.0-rc6-next-20220921+ #72 > [ 6939.874562] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.15.0-1.fc35 04/01/2014 > [ 6939.877707] Call Trace: > [ 6939.878745] <TASK> > [ 6939.879779] dump_stack_lvl+0x6c/0x9f > [ 6939.881199] handle_userfault.cold+0x14/0x1e > [ 6939.882830] ? find_held_lock+0x2b/0x80 > [ 6939.884370] ? __mutex_unlock_slowpath+0x45/0x280 > [ 6939.886145] hugetlb_handle_userfault+0x90/0xf0 > [ 6939.887936] hugetlb_fault+0xb7e/0xda0 > [ 6939.889409] ? vprintk_emit+0x118/0x3a0 > [ 6939.890903] ? _printk+0x58/0x73 > [ 6939.892279] follow_hugetlb_page.cold+0x59/0x145 > [ 6939.894116] __get_user_pages+0x146/0x750 > [ 6939.895580] __gup_longterm_locked+0x3e9/0x680 > [ 6939.897023] ? seqcount_lockdep_reader_access.constprop.0+0xa5/0xb0 > [ 6939.898939] ? lockdep_hardirqs_on+0x7d/0x100 > [ 6939.901243] gup_test_ioctl+0x320/0x6e0 > [ 6939.902202] __x64_sys_ioctl+0x87/0xc0 > [ 6939.903220] do_syscall_64+0x38/0x90 > [ 6939.904233] entry_SYSCALL_64_after_hwframe+0x63/0xcd > [ 6939.905423] RIP: 0033:0x7fbb53830f7b > > This is because userfaultfd is expecting FAULT_FLAG_ALLOW_RETRY which is not > set in this path. > > Adding John, Peter and David on Cc: as they are much more fluent in all the > fault and FOLL combinations and might have immediate suggestions. It is going > to take me a little while to figure out: > 1) How to make sure we get the right flags passed to handle_userfault As David mentioned, one way is to have "locked" passed in with non-NULL. The other way is to have FOLL_NOWAIT even if locked==NULL. Here IIUC the trick is when the GUP caller neither wants to release the mmap lock, nor does it want to stop quickly (i.e. it wants to wait for the page fault with mmap lock held), then we'll have both locked==NULL and !FOLL_NOWAIT. Userfaultfd currently doesn't think it's wise so generated that warning with CONFIG_DEBUG_VM. > 2) How to modify follow_hugetlb_page as userfaultfd can certainly drop > mmap_lock. So we can not assume vma still exists upon return. I think FOLL_NOWAIT flag might work if the only thing we want to do is to trigger handle_userfault() path. But I'll also look into vm/userfaultfd as mentioned above to make sure we'll have GUP covered there too. I'll update if I found anything useful there. Off-topic a bit: the whole discussion reminded me something on whether userfaultfd is doing correctly here. E.g., here userfaultfd should really look like the case when a swap in is needed for a file. FOLL_NOWAIT on swap-in will mean: #define FOLL_NOWAIT 0x20 /* if a disk transfer is needed, start the IO * and return without waiting upon it */ Now userfaultfd returns VM_FAULT_RETRY immediately with FOLL_NOWAIT. I'm wondering whether it should really generate the message before doing that, to match with the semantics of initial use of FOLL_NOWAIT on swapping. Thanks,
On 09/22/22 09:46, David Hildenbrand wrote: > On 22.09.22 01:57, Mike Kravetz wrote: > > On 09/21/22 10:48, Mike Kravetz wrote: > > > On 09/21/22 16:34, 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. > > > > > > Thank you very much! > > > > > > When I saw this report, the obvious fix was to do something like what you have > > > done below. That looks fine with a few minor comments. > > > > > > One question I have not yet answered is, "Does this same issue apply to > > > follow_hugetlb_page()?". I believe it does. follow_hugetlb_page calls > > > hugetlb_fault which could result in the fault being processed by userfaultfd. > > > If we experience the race above, then the associated vma could no longer be > > > valid when returning from hugetlb_fault. follow_hugetlb_page and callers > > > have a flag (locked) to deal with dropping mmap lock. However, I am not sure > > > if it is handled correctly WRT userfaultfd. I think this needs to be answered > > > before fixing. And, if the follow_hugetlb_page code needs to be fixed it > > > should be done at the same time. > > > > > > > To at least verify this code path, I added userfaultfd handling to the gup_test > > program in kernel selftests. When doing basic gup test on a hugetlb page in > > a userfaultfd registered range, I hit this warning: > > > > [ 6939.867796] FAULT_FLAG_ALLOW_RETRY missing 1 > > [ 6939.871503] CPU: 2 PID: 5720 Comm: gup_test Not tainted 6.0.0-rc6-next-20220921+ #72 > > [ 6939.874562] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.15.0-1.fc35 04/01/2014 > > [ 6939.877707] Call Trace: > > [ 6939.878745] <TASK> > > [ 6939.879779] dump_stack_lvl+0x6c/0x9f > > [ 6939.881199] handle_userfault.cold+0x14/0x1e > > [ 6939.882830] ? find_held_lock+0x2b/0x80 > > [ 6939.884370] ? __mutex_unlock_slowpath+0x45/0x280 > > [ 6939.886145] hugetlb_handle_userfault+0x90/0xf0 > > [ 6939.887936] hugetlb_fault+0xb7e/0xda0 > > [ 6939.889409] ? vprintk_emit+0x118/0x3a0 > > [ 6939.890903] ? _printk+0x58/0x73 > > [ 6939.892279] follow_hugetlb_page.cold+0x59/0x145 > > [ 6939.894116] __get_user_pages+0x146/0x750 > > [ 6939.895580] __gup_longterm_locked+0x3e9/0x680 > > [ 6939.897023] ? seqcount_lockdep_reader_access.constprop.0+0xa5/0xb0 > > [ 6939.898939] ? lockdep_hardirqs_on+0x7d/0x100 > > [ 6939.901243] gup_test_ioctl+0x320/0x6e0 > > [ 6939.902202] __x64_sys_ioctl+0x87/0xc0 > > [ 6939.903220] do_syscall_64+0x38/0x90 > > [ 6939.904233] entry_SYSCALL_64_after_hwframe+0x63/0xcd > > [ 6939.905423] RIP: 0033:0x7fbb53830f7b > > > > This is because userfaultfd is expecting FAULT_FLAG_ALLOW_RETRY which is not > > set in this path. > > Right. Without being able to drop the mmap lock, we cannot continue. And we > don't know if we can drop it without FAULT_FLAG_ALLOW_RETRY. > > FAULT_FLAG_ALLOW_RETRY is only set when we can communicate to the caller > that we dropped the mmap lock [e.g., int *locked parameter]. > > All code paths that pass NULL won't be able to handle -- especially > surprisingly also pin_user_pages_fast() -- cannot trigger usefaultfd and > will result in this warning. > > > A "sane" example is access via /proc/self/mem via ptrace: we don't want to > trigger userfaultfd, but instead simply fail the GUP get/pin. > > > Now, this is just a printed *warning* (not a WARN/BUG/taint) that tells us > that there is a GUP user that isn't prepared for userfaultfd. So it rather > points out a missing GUP adaption -- incomplete userfaultfd support. And we > seem to have plenty of that judging that pin_user_pages_fast_only(). > > Maybe the printed stack trace is a bit too much and makes this look very > scary. > > > > > Adding John, Peter and David on Cc: as they are much more fluent in all the > > fault and FOLL combinations and might have immediate suggestions. It is going > > to take me a little while to figure out: > > 1) How to make sure we get the right flags passed to handle_userfault > > This is a GUP caller problem -- or rather, how GUP has to deal with > userfaultfd. > > > 2) How to modify follow_hugetlb_page as userfaultfd can certainly drop > > mmap_lock. So we can not assume vma still exists upon return. > > Again, we have to communicate to the GUP caller that we dropped the mmap > lock. And that requires GUP caller changes. > Thank you and Peter for replying! The 'good news' is that there does not appear to be a case where userfaultfd (via hugetlb_fault) drops the lock and follow_hugetlb_page is not prepard for the consequences. So, this is not an exposure as in hugetlb_handle_userfault that is in need of an immediate fix. i.e. A fix like that originally proposed here is sufficient. We can think about whether this specific calling sequence needs to be modified.
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;
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(-)