| Message ID | 8343cd77ca301df15839796f3b446b75ce5ffbbf.1550839937.git.andreyknvl@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: untag user pointers passed to the kernel | expand |
On 2/22/19 4:53 AM, Andrey Konovalov wrote: > userfaultfd_register() and userfaultfd_unregister() use provided user > pointers for vma lookups, which can only by done with untagged pointers. So, we have to patch all these sites before the tagged values get to the point of hitting the vma lookup functions. Dumb question: Why don't we just patch the vma lookup functions themselves instead of all of these callers?
On Sat, Feb 23, 2019 at 12:06 AM Dave Hansen <dave.hansen@intel.com> wrote: > > On 2/22/19 4:53 AM, Andrey Konovalov wrote: > > userfaultfd_register() and userfaultfd_unregister() use provided user > > pointers for vma lookups, which can only by done with untagged pointers. > > So, we have to patch all these sites before the tagged values get to the > point of hitting the vma lookup functions. Dumb question: Why don't we > just patch the vma lookup functions themselves instead of all of these > callers? That might be a working approach as well. We'll still need to fix up places where the vma fields are accessed directly. Catalin, what do you think?
On Tue, Feb 26, 2019 at 03:39:08PM +0100, Andrey Konovalov wrote: > On Sat, Feb 23, 2019 at 12:06 AM Dave Hansen <dave.hansen@intel.com> wrote: > > > > On 2/22/19 4:53 AM, Andrey Konovalov wrote: > > > userfaultfd_register() and userfaultfd_unregister() use provided user > > > pointers for vma lookups, which can only by done with untagged pointers. > > > > So, we have to patch all these sites before the tagged values get to the > > point of hitting the vma lookup functions. Dumb question: Why don't we > > just patch the vma lookup functions themselves instead of all of these > > callers? > > That might be a working approach as well. We'll still need to fix up > places where the vma fields are accessed directly. Catalin, what do > you think? Most callers of find_vma*() always follow it by a check of vma->vma_start against some tagged address ('end' in the userfaultfd_(un)register()) case. So it's not sufficient to untag it in find_vma().
On 3/1/19 8:59 AM, Catalin Marinas wrote: >>> So, we have to patch all these sites before the tagged values get to the >>> point of hitting the vma lookup functions. Dumb question: Why don't we >>> just patch the vma lookup functions themselves instead of all of these >>> callers? >> That might be a working approach as well. We'll still need to fix up >> places where the vma fields are accessed directly. Catalin, what do >> you think? > Most callers of find_vma*() always follow it by a check of > vma->vma_start against some tagged address ('end' in the > userfaultfd_(un)register()) case. So it's not sufficient to untag it in > find_vma(). If that's truly the common case, sounds like we should have a find_vma() that does the vma_end checking as well. Then at least the common case would not have to worry about tagging.
On Fri, Mar 1, 2019 at 7:37 PM Dave Hansen <dave.hansen@intel.com> wrote: > > On 3/1/19 8:59 AM, Catalin Marinas wrote: > >>> So, we have to patch all these sites before the tagged values get to the > >>> point of hitting the vma lookup functions. Dumb question: Why don't we > >>> just patch the vma lookup functions themselves instead of all of these > >>> callers? > >> That might be a working approach as well. We'll still need to fix up > >> places where the vma fields are accessed directly. Catalin, what do > >> you think? > > Most callers of find_vma*() always follow it by a check of > > vma->vma_start against some tagged address ('end' in the > > userfaultfd_(un)register()) case. So it's not sufficient to untag it in > > find_vma(). > > If that's truly the common case, sounds like we should have a find_vma() > that does the vma_end checking as well. Then at least the common case > would not have to worry about tagging. It seems that a lot of find_vma() callers indeed do different kinds of checking/subtractions of vma->vma_start and a tagged address, which look hardly unifiable. So untagging the addresses in find_vma() callers looks like a more suitable solution.
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c index 89800fc7dc9d..a3b70e0d9756 100644 --- a/fs/userfaultfd.c +++ b/fs/userfaultfd.c @@ -1320,6 +1320,9 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx, goto out; } + uffdio_register.range.start = + untagged_addr(uffdio_register.range.start); + ret = validate_range(mm, uffdio_register.range.start, uffdio_register.range.len); if (ret) @@ -1507,6 +1510,8 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx, if (copy_from_user(&uffdio_unregister, buf, sizeof(uffdio_unregister))) goto out; + uffdio_unregister.start = untagged_addr(uffdio_unregister.start); + ret = validate_range(mm, uffdio_unregister.start, uffdio_unregister.len); if (ret)
userfaultfd_register() and userfaultfd_unregister() use provided user pointers for vma lookups, which can only by done with untagged pointers. Untag user pointers in these functions. Signed-off-by: Andrey Konovalov <andreyknvl@google.com> --- fs/userfaultfd.c | 5 +++++ 1 file changed, 5 insertions(+)