| Message ID | 20240125164256.4147-31-alexandru.elisei@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for arm64 MTE dynamic tag storage reuse | expand |
On 1/25/24 22:12, Alexandru Elisei wrote: > A page can end up mapped in a MTE enabled VMA without the corresponding tag > storage block reserved. Tag accesses made by ptrace in this case can lead > to the wrong tags being read or memory corruption for the process that is > using the tag storage memory as data. > > Reserve tag storage by treating ptrace accesses like a fault. > > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> > --- > > Changes since rfc v2: > > * New patch, issue reported by Peter Collingbourne. > > arch/arm64/kernel/mte.c | 26 ++++++++++++++++++++++++-- > 1 file changed, 24 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c > index faf09da3400a..b1fa02dad4fd 100644 > --- a/arch/arm64/kernel/mte.c > +++ b/arch/arm64/kernel/mte.c > @@ -412,10 +412,13 @@ static int __access_remote_tags(struct mm_struct *mm, unsigned long addr, > while (len) { > struct vm_area_struct *vma; > unsigned long tags, offset; > + unsigned int fault_flags; > + struct page *page; > + vm_fault_t ret; > void *maddr; > - struct page *page = get_user_page_vma_remote(mm, addr, > - gup_flags, &vma); > > +get_page: > + page = get_user_page_vma_remote(mm, addr, gup_flags, &vma); But if there is valid page returned here in the first GUP attempt, will there still be a subsequent handle_mm_fault() on the same vma and addr ? > if (IS_ERR(page)) { > err = PTR_ERR(page); > break; > @@ -433,6 +436,25 @@ static int __access_remote_tags(struct mm_struct *mm, unsigned long addr, > put_page(page); > break; > } > + > + if (tag_storage_enabled() && !page_tag_storage_reserved(page)) { Should not '!page' be checked here as well ? > + fault_flags = FAULT_FLAG_DEFAULT | \ > + FAULT_FLAG_USER | \ > + FAULT_FLAG_REMOTE | \ > + FAULT_FLAG_ALLOW_RETRY | \ > + FAULT_FLAG_RETRY_NOWAIT; > + if (write) > + fault_flags |= FAULT_FLAG_WRITE; > + > + put_page(page); > + ret = handle_mm_fault(vma, addr, fault_flags, NULL); > + if (ret & VM_FAULT_ERROR) { > + err = -EFAULT; > + break; > + } > + goto get_page; > + } > + > WARN_ON_ONCE(!page_mte_tagged(page)); > > /* limit access to the end of the page */
Hi, On Thu, Feb 01, 2024 at 02:51:39PM +0530, Anshuman Khandual wrote: > > > On 1/25/24 22:12, Alexandru Elisei wrote: > > A page can end up mapped in a MTE enabled VMA without the corresponding tag > > storage block reserved. Tag accesses made by ptrace in this case can lead > > to the wrong tags being read or memory corruption for the process that is > > using the tag storage memory as data. > > > > Reserve tag storage by treating ptrace accesses like a fault. > > > > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> > > --- > > > > Changes since rfc v2: > > > > * New patch, issue reported by Peter Collingbourne. > > > > arch/arm64/kernel/mte.c | 26 ++++++++++++++++++++++++-- > > 1 file changed, 24 insertions(+), 2 deletions(-) > > > > diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c > > index faf09da3400a..b1fa02dad4fd 100644 > > --- a/arch/arm64/kernel/mte.c > > +++ b/arch/arm64/kernel/mte.c > > @@ -412,10 +412,13 @@ static int __access_remote_tags(struct mm_struct *mm, unsigned long addr, > > while (len) { > > struct vm_area_struct *vma; > > unsigned long tags, offset; > > + unsigned int fault_flags; > > + struct page *page; > > + vm_fault_t ret; > > void *maddr; > > - struct page *page = get_user_page_vma_remote(mm, addr, > > - gup_flags, &vma); > > > > +get_page: > > + page = get_user_page_vma_remote(mm, addr, gup_flags, &vma); > > But if there is valid page returned here in the first GUP attempt, will there > still be a subsequent handle_mm_fault() on the same vma and addr ? Only if it's missing tag storage. If it's missing tag storage, the page has been mapped as arch_fault_on_access_pte(), and handle_mm_fault()->..->arch_handle_folio_fault_on_access() will either reserve tag storage, or migrate it. > > > if (IS_ERR(page)) { > > err = PTR_ERR(page); > > break; > > @@ -433,6 +436,25 @@ static int __access_remote_tags(struct mm_struct *mm, unsigned long addr, > > put_page(page); > > break; > > } > > + > > + if (tag_storage_enabled() && !page_tag_storage_reserved(page)) { > > Should not '!page' be checked here as well ? I was under the impression that get_user_page_vma_remote() returns an error pointer if gup couldn't pin the page. Thanks, Alex > > > + fault_flags = FAULT_FLAG_DEFAULT | \ > > + FAULT_FLAG_USER | \ > > + FAULT_FLAG_REMOTE | \ > > + FAULT_FLAG_ALLOW_RETRY | \ > > + FAULT_FLAG_RETRY_NOWAIT; > > + if (write) > > + fault_flags |= FAULT_FLAG_WRITE; > > + > > + put_page(page); > > + ret = handle_mm_fault(vma, addr, fault_flags, NULL); > > + if (ret & VM_FAULT_ERROR) { > > + err = -EFAULT; > > + break; > > + } > > + goto get_page; > > + } > > + > > WARN_ON_ONCE(!page_mte_tagged(page)); > > > > /* limit access to the end of the page */
diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c index faf09da3400a..b1fa02dad4fd 100644 --- a/arch/arm64/kernel/mte.c +++ b/arch/arm64/kernel/mte.c @@ -412,10 +412,13 @@ static int __access_remote_tags(struct mm_struct *mm, unsigned long addr, while (len) { struct vm_area_struct *vma; unsigned long tags, offset; + unsigned int fault_flags; + struct page *page; + vm_fault_t ret; void *maddr; - struct page *page = get_user_page_vma_remote(mm, addr, - gup_flags, &vma); +get_page: + page = get_user_page_vma_remote(mm, addr, gup_flags, &vma); if (IS_ERR(page)) { err = PTR_ERR(page); break; @@ -433,6 +436,25 @@ static int __access_remote_tags(struct mm_struct *mm, unsigned long addr, put_page(page); break; } + + if (tag_storage_enabled() && !page_tag_storage_reserved(page)) { + fault_flags = FAULT_FLAG_DEFAULT | \ + FAULT_FLAG_USER | \ + FAULT_FLAG_REMOTE | \ + FAULT_FLAG_ALLOW_RETRY | \ + FAULT_FLAG_RETRY_NOWAIT; + if (write) + fault_flags |= FAULT_FLAG_WRITE; + + put_page(page); + ret = handle_mm_fault(vma, addr, fault_flags, NULL); + if (ret & VM_FAULT_ERROR) { + err = -EFAULT; + break; + } + goto get_page; + } + WARN_ON_ONCE(!page_mte_tagged(page)); /* limit access to the end of the page */
A page can end up mapped in a MTE enabled VMA without the corresponding tag storage block reserved. Tag accesses made by ptrace in this case can lead to the wrong tags being read or memory corruption for the process that is using the tag storage memory as data. Reserve tag storage by treating ptrace accesses like a fault. Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> --- Changes since rfc v2: * New patch, issue reported by Peter Collingbourne. arch/arm64/kernel/mte.c | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-)