| Message ID | 0db34d04fa16be162336106e3b4a94f3dacc0af4.1524077494.git.andreyknvl@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 18, 2018 at 08:53:13PM +0200, Andrey Konovalov wrote: > diff --git a/mm/gup.c b/mm/gup.c > index 76af4cfeaf68..fb375de7d40d 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -386,6 +386,8 @@ struct page *follow_page_mask(struct vm_area_struct *vma, > struct page *page; > struct mm_struct *mm = vma->vm_mm; > > + address = untagged_addr(address); > + > *page_mask = 0; > > /* make this handle hugepd */ Does having a tagged address here makes any difference? I couldn't hit a failure with my simple tests (LD_PRELOAD a library that randomly adds tags to pointers returned by malloc). > @@ -647,6 +649,8 @@ static long __get_user_pages(struct task_struct *tsk, struct mm_struct *mm, > if (!nr_pages) > return 0; > > + start = untagged_addr(start); > + > VM_BUG_ON(!!pages != !!(gup_flags & FOLL_GET)); > > /* > @@ -801,6 +805,8 @@ int fixup_user_fault(struct task_struct *tsk, struct mm_struct *mm, > struct vm_area_struct *vma; > int ret, major = 0; > > + address = untagged_addr(address); > + > if (unlocked) > fault_flags |= FAULT_FLAG_ALLOW_RETRY; > > @@ -854,6 +860,8 @@ static __always_inline long __get_user_pages_locked(struct task_struct *tsk, > long ret, pages_done; > bool lock_dropped; > > + start = untagged_addr(start); > + > if (locked) { > /* if VM_FAULT_RETRY can be returned, vmas become invalid */ > BUG_ON(vmas); Isn't __get_user_pages() untagging enough to cover this case as well? Can this function not cope with tagged pointers? > @@ -1751,6 +1759,8 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write, > unsigned long flags; > int nr = 0; > > + start = untagged_addr(start); > + > start &= PAGE_MASK; > addr = start; > len = (unsigned long) nr_pages << PAGE_SHIFT; > @@ -1803,6 +1813,8 @@ int get_user_pages_fast(unsigned long start, int nr_pages, int write, > unsigned long addr, len, end; > int nr = 0, ret = 0; > > + start = untagged_addr(start); > + > start &= PAGE_MASK; > addr = start; > len = (unsigned long) nr_pages << PAGE_SHIFT; Have you hit a problem with the fast gup functions and tagged pointers? The page table walking macros (e.g. p*d_index()) should mask the tag out already.
On Thu, Apr 26, 2018 at 7:47 PM, Catalin Marinas <catalin.marinas@arm.com> wrote: My approach with this was to add untagging to every gup.c function that is exposed for external use, but perhaps adding untagging only where it is actually required is a better approach. > On Wed, Apr 18, 2018 at 08:53:13PM +0200, Andrey Konovalov wrote: >> diff --git a/mm/gup.c b/mm/gup.c >> index 76af4cfeaf68..fb375de7d40d 100644 >> --- a/mm/gup.c >> +++ b/mm/gup.c >> @@ -386,6 +386,8 @@ struct page *follow_page_mask(struct vm_area_struct *vma, >> struct page *page; >> struct mm_struct *mm = vma->vm_mm; >> >> + address = untagged_addr(address); >> + >> *page_mask = 0; >> >> /* make this handle hugepd */ > > Does having a tagged address here makes any difference? I couldn't hit a > failure with my simple tests (LD_PRELOAD a library that randomly adds > tags to pointers returned by malloc). I think you're right, follow_page_mask is only called from __get_user_pages, which already untagged the address. I'll remove untagging here. > >> @@ -647,6 +649,8 @@ static long __get_user_pages(struct task_struct *tsk, struct mm_struct *mm, >> if (!nr_pages) >> return 0; >> >> + start = untagged_addr(start); >> + >> VM_BUG_ON(!!pages != !!(gup_flags & FOLL_GET)); >> >> /* >> @@ -801,6 +805,8 @@ int fixup_user_fault(struct task_struct *tsk, struct mm_struct *mm, >> struct vm_area_struct *vma; >> int ret, major = 0; >> >> + address = untagged_addr(address); >> + >> if (unlocked) >> fault_flags |= FAULT_FLAG_ALLOW_RETRY; >> >> @@ -854,6 +860,8 @@ static __always_inline long __get_user_pages_locked(struct task_struct *tsk, >> long ret, pages_done; >> bool lock_dropped; >> >> + start = untagged_addr(start); >> + >> if (locked) { >> /* if VM_FAULT_RETRY can be returned, vmas become invalid */ >> BUG_ON(vmas); > > Isn't __get_user_pages() untagging enough to cover this case as well? > Can this function not cope with tagged pointers? Yes, I think you're right here as well. I'll remove untagging here. > >> @@ -1751,6 +1759,8 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write, >> unsigned long flags; >> int nr = 0; >> >> + start = untagged_addr(start); >> + >> start &= PAGE_MASK; >> addr = start; >> len = (unsigned long) nr_pages << PAGE_SHIFT; >> @@ -1803,6 +1813,8 @@ int get_user_pages_fast(unsigned long start, int nr_pages, int write, >> unsigned long addr, len, end; >> int nr = 0, ret = 0; >> >> + start = untagged_addr(start); >> + >> start &= PAGE_MASK; >> addr = start; >> len = (unsigned long) nr_pages << PAGE_SHIFT; > > Have you hit a problem with the fast gup functions and tagged pointers? > The page table walking macros (e.g. p*d_index()) should mask the tag out > already. I didn't hit a problem, but the plan was to add untagging to all gup.c interface functions as I mentioned above. Here get_user_pages_fast can cope with tagged addresses as long as gup_pgd_range can. And looks like the latter can indeed do that since it only uses addr through the page table walking macros you mentioned. I'll remove untagging here as well. Thanks!
On Wed, May 02, 2018 at 02:38:42PM +0000, Andrey Konovalov wrote: > > Does having a tagged address here makes any difference? I couldn't hit a > > failure with my simple tests (LD_PRELOAD a library that randomly adds > > tags to pointers returned by malloc). > > I think you're right, follow_page_mask is only called from > __get_user_pages, which already untagged the address. I'll remove > untagging here. It also called from follow_page(). Have you covered all its callers?
On Wed, May 2, 2018 at 5:36 PM, Kirill A. Shutemov <kirill.shutemov@linux.intel.com> wrote: > On Wed, May 02, 2018 at 02:38:42PM +0000, Andrey Konovalov wrote: >> > Does having a tagged address here makes any difference? I couldn't hit a >> > failure with my simple tests (LD_PRELOAD a library that randomly adds >> > tags to pointers returned by malloc). >> >> I think you're right, follow_page_mask is only called from >> __get_user_pages, which already untagged the address. I'll remove >> untagging here. > > It also called from follow_page(). Have you covered all its callers? Oh, missed that, will take a look. Thinking about that, would it make sense to add untagging to find_vma (and others) instead of trying to cover all find_vma callers?
On Wed, May 2, 2018 at 7:25 PM, Andrey Konovalov <andreyknvl@google.com> wrote: > On Wed, May 2, 2018 at 5:36 PM, Kirill A. Shutemov > <kirill.shutemov@linux.intel.com> wrote: >> On Wed, May 02, 2018 at 02:38:42PM +0000, Andrey Konovalov wrote: >>> > Does having a tagged address here makes any difference? I couldn't hit a >>> > failure with my simple tests (LD_PRELOAD a library that randomly adds >>> > tags to pointers returned by malloc). >>> >>> I think you're right, follow_page_mask is only called from >>> __get_user_pages, which already untagged the address. I'll remove >>> untagging here. >> >> It also called from follow_page(). Have you covered all its callers? > > Oh, missed that, will take a look. I wasn't able to find anything that calls follow_page with pointers passed from userspace except for the memory subsystem syscalls, and we deliberately don't add untagging in those.
On Thu, May 03, 2018 at 04:09:56PM +0200, Andrey Konovalov wrote: > On Wed, May 2, 2018 at 7:25 PM, Andrey Konovalov <andreyknvl@google.com> wrote: > > On Wed, May 2, 2018 at 5:36 PM, Kirill A. Shutemov > > <kirill.shutemov@linux.intel.com> wrote: > >> On Wed, May 02, 2018 at 02:38:42PM +0000, Andrey Konovalov wrote: > >>> > Does having a tagged address here makes any difference? I couldn't hit a > >>> > failure with my simple tests (LD_PRELOAD a library that randomly adds > >>> > tags to pointers returned by malloc). > >>> > >>> I think you're right, follow_page_mask is only called from > >>> __get_user_pages, which already untagged the address. I'll remove > >>> untagging here. > >> > >> It also called from follow_page(). Have you covered all its callers? > > > > Oh, missed that, will take a look. > > I wasn't able to find anything that calls follow_page with pointers > passed from userspace except for the memory subsystem syscalls, and we > deliberately don't add untagging in those. I guess I missed this part, but could you elaborate on this? Why? Not yet or not ever? Also I wounder if we can find (with sparse?) all places where we cast out __user. This would give a nice list of places where to pay attention.
On Thu, May 3, 2018 at 5:24 PM, Kirill A. Shutemov <kirill@shutemov.name> wrote: > On Thu, May 03, 2018 at 04:09:56PM +0200, Andrey Konovalov wrote: >> On Wed, May 2, 2018 at 7:25 PM, Andrey Konovalov <andreyknvl@google.com> wrote: >> I wasn't able to find anything that calls follow_page with pointers >> passed from userspace except for the memory subsystem syscalls, and we >> deliberately don't add untagging in those. > > I guess I missed this part, but could you elaborate on this? Why? > Not yet or not ever? Check out the discussion here: https://www.spinics.net/lists/arm-kernel/msg640936.html > > Also I wounder if we can find (with sparse?) all places where we cast out > __user. This would give a nice list of places where to pay attention. The way I tested this is I added BUG_ON(top byte tag is set) to find_vma and find_extend_vma and ran a modified version of syzkaller that embeds tags into pointers overnight. The only crashes that I saw were coming from memory subsystem syscalls. I then temporarily added untagging to suppress those crashes (https://gist.github.com/xairy/3aa1f57798fa62522c8ac53fad9b74ca), and didn't see any crashes after that.
On Wed, May 02, 2018 at 07:25:17PM +0200, Andrey Konovalov wrote: > On Wed, May 2, 2018 at 5:36 PM, Kirill A. Shutemov > <kirill.shutemov@linux.intel.com> wrote: > > On Wed, May 02, 2018 at 02:38:42PM +0000, Andrey Konovalov wrote: > >> > Does having a tagged address here makes any difference? I couldn't hit a > >> > failure with my simple tests (LD_PRELOAD a library that randomly adds > >> > tags to pointers returned by malloc). > >> > >> I think you're right, follow_page_mask is only called from > >> __get_user_pages, which already untagged the address. I'll remove > >> untagging here. > > > > It also called from follow_page(). Have you covered all its callers? > > Oh, missed that, will take a look. > > Thinking about that, would it make sense to add untagging to find_vma > (and others) instead of trying to cover all find_vma callers? I don't think adding the untagging to find_vma() is sufficient. In many cases the caller does a subsequent check like 'start < vma->vm_start' (see sys_msync() as an example, there are a few others as well). What I did in my tests was a WARN_ON_ONCE() in find_vma() if the address is tagged.
On Tue, May 8, 2018 at 5:11 PM, Catalin Marinas <catalin.marinas@arm.com> wrote: > On Wed, May 02, 2018 at 07:25:17PM +0200, Andrey Konovalov wrote: >> On Wed, May 2, 2018 at 5:36 PM, Kirill A. Shutemov >> <kirill.shutemov@linux.intel.com> wrote: >> > On Wed, May 02, 2018 at 02:38:42PM +0000, Andrey Konovalov wrote: >> >> > Does having a tagged address here makes any difference? I couldn't hit a >> >> > failure with my simple tests (LD_PRELOAD a library that randomly adds >> >> > tags to pointers returned by malloc). >> >> >> >> I think you're right, follow_page_mask is only called from >> >> __get_user_pages, which already untagged the address. I'll remove >> >> untagging here. >> > >> > It also called from follow_page(). Have you covered all its callers? >> >> Oh, missed that, will take a look. >> >> Thinking about that, would it make sense to add untagging to find_vma >> (and others) instead of trying to cover all find_vma callers? > > I don't think adding the untagging to find_vma() is sufficient. In many > cases the caller does a subsequent check like 'start < vma->vm_start' > (see sys_msync() as an example, there are a few others as well). OK. > What I > did in my tests was a WARN_ON_ONCE() in find_vma() if the address is > tagged. So this is similar to what I did. Do you think trying to find "all places where we cast out __user" with static analysis as Kirill suggested is something I should pursue? Or is this patchset is good as is as the first approximation, since we can fix more things where untagging is needed as we discover them one by one?
diff --git a/mm/gup.c b/mm/gup.c index 76af4cfeaf68..fb375de7d40d 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -386,6 +386,8 @@ struct page *follow_page_mask(struct vm_area_struct *vma, struct page *page; struct mm_struct *mm = vma->vm_mm; + address = untagged_addr(address); + *page_mask = 0; /* make this handle hugepd */ @@ -647,6 +649,8 @@ static long __get_user_pages(struct task_struct *tsk, struct mm_struct *mm, if (!nr_pages) return 0; + start = untagged_addr(start); + VM_BUG_ON(!!pages != !!(gup_flags & FOLL_GET)); /* @@ -801,6 +805,8 @@ int fixup_user_fault(struct task_struct *tsk, struct mm_struct *mm, struct vm_area_struct *vma; int ret, major = 0; + address = untagged_addr(address); + if (unlocked) fault_flags |= FAULT_FLAG_ALLOW_RETRY; @@ -854,6 +860,8 @@ static __always_inline long __get_user_pages_locked(struct task_struct *tsk, long ret, pages_done; bool lock_dropped; + start = untagged_addr(start); + if (locked) { /* if VM_FAULT_RETRY can be returned, vmas become invalid */ BUG_ON(vmas); @@ -1751,6 +1759,8 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write, unsigned long flags; int nr = 0; + start = untagged_addr(start); + start &= PAGE_MASK; addr = start; len = (unsigned long) nr_pages << PAGE_SHIFT; @@ -1803,6 +1813,8 @@ int get_user_pages_fast(unsigned long start, int nr_pages, int write, unsigned long addr, len, end; int nr = 0, ret = 0; + start = untagged_addr(start); + start &= PAGE_MASK; addr = start; len = (unsigned long) nr_pages << PAGE_SHIFT;
mm/gup.c provides a kernel interface that accepts user addresses and manipulates user pages directly (for example get_user_pages, that is used by the futex syscall). Here we also need to handle the case of tagged user pointers. Untag addresses passed to this interface. Signed-off-by: Andrey Konovalov <andreyknvl@google.com> --- mm/gup.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)