| Message ID | 44ad2d0c55dbad449edac23ae46d151a04102a1d.1553093421.git.andreyknvl@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: untag user pointers passed to the kernel | expand |
On Wed, Mar 20, 2019 at 03:51:18PM +0100, Andrey Konovalov wrote: > This patch is a part of a series that extends arm64 kernel ABI to allow to > pass tagged user pointers (with the top byte set to something else other > than 0x00) as syscall arguments. > > This patch allows tagged pointers to be passed to the following memory > syscalls: madvise, mbind, get_mempolicy, mincore, mlock, mlock2, brk, > mmap_pgoff, old_mmap, munmap, remap_file_pages, mprotect, pkey_mprotect, > mremap, msync and shmdt. > > This is done by untagging pointers passed to these syscalls in the > prologues of their handlers. > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> > --- > ipc/shm.c | 2 ++ > mm/madvise.c | 2 ++ > mm/mempolicy.c | 5 +++++ > mm/migrate.c | 1 + > mm/mincore.c | 2 ++ > mm/mlock.c | 5 +++++ > mm/mmap.c | 7 +++++++ > mm/mprotect.c | 1 + > mm/mremap.c | 2 ++ > mm/msync.c | 2 ++ > 10 files changed, 29 insertions(+) I wonder whether it's better to keep these as wrappers in the arm64 code.
On Fri, Mar 22, 2019 at 12:44 PM Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Wed, Mar 20, 2019 at 03:51:18PM +0100, Andrey Konovalov wrote: > > This patch is a part of a series that extends arm64 kernel ABI to allow to > > pass tagged user pointers (with the top byte set to something else other > > than 0x00) as syscall arguments. > > > > This patch allows tagged pointers to be passed to the following memory > > syscalls: madvise, mbind, get_mempolicy, mincore, mlock, mlock2, brk, > > mmap_pgoff, old_mmap, munmap, remap_file_pages, mprotect, pkey_mprotect, > > mremap, msync and shmdt. > > > > This is done by untagging pointers passed to these syscalls in the > > prologues of their handlers. > > > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> > > --- > > ipc/shm.c | 2 ++ > > mm/madvise.c | 2 ++ > > mm/mempolicy.c | 5 +++++ > > mm/migrate.c | 1 + > > mm/mincore.c | 2 ++ > > mm/mlock.c | 5 +++++ > > mm/mmap.c | 7 +++++++ > > mm/mprotect.c | 1 + > > mm/mremap.c | 2 ++ > > mm/msync.c | 2 ++ > > 10 files changed, 29 insertions(+) > > I wonder whether it's better to keep these as wrappers in the arm64 > code. I don't think I understand what you propose, could you elaborate?
On Thu, 28 Mar 2019 19:10:07 +0100 Andrey Konovalov <andreyknvl@google.com> wrote: > > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> > > > --- > > > ipc/shm.c | 2 ++ > > > mm/madvise.c | 2 ++ > > > mm/mempolicy.c | 5 +++++ > > > mm/migrate.c | 1 + > > > mm/mincore.c | 2 ++ > > > mm/mlock.c | 5 +++++ > > > mm/mmap.c | 7 +++++++ > > > mm/mprotect.c | 1 + > > > mm/mremap.c | 2 ++ > > > mm/msync.c | 2 ++ > > > 10 files changed, 29 insertions(+) > > > > I wonder whether it's better to keep these as wrappers in the arm64 > > code. > > I don't think I understand what you propose, could you elaborate? I believe Catalin is saying that instead of placing things like: @@ -1593,6 +1593,7 @@ SYSCALL_DEFINE3(shmat, int, shmid, char __user *, shmaddr, int, shmflg) unsigned long ret; long err; + shmaddr = untagged_addr(shmaddr); To instead have the shmaddr set to the untagged_addr() before calling the system call, and passing the untagged addr to the system call, as that goes through the arm64 architecture specific code first. -- Steve
(I trimmed down the cc list a bit since it's always bouncing) On Thu, Mar 28, 2019 at 02:19:34PM -0400, Steven Rostedt wrote: > On Thu, 28 Mar 2019 19:10:07 +0100 > Andrey Konovalov <andreyknvl@google.com> wrote: > > > > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> > > > > --- > > > > ipc/shm.c | 2 ++ > > > > mm/madvise.c | 2 ++ > > > > mm/mempolicy.c | 5 +++++ > > > > mm/migrate.c | 1 + > > > > mm/mincore.c | 2 ++ > > > > mm/mlock.c | 5 +++++ > > > > mm/mmap.c | 7 +++++++ > > > > mm/mprotect.c | 1 + > > > > mm/mremap.c | 2 ++ > > > > mm/msync.c | 2 ++ > > > > 10 files changed, 29 insertions(+) > > > > > > I wonder whether it's better to keep these as wrappers in the arm64 > > > code. > > > > I don't think I understand what you propose, could you elaborate? > > I believe Catalin is saying that instead of placing things like: > > @@ -1593,6 +1593,7 @@ SYSCALL_DEFINE3(shmat, int, shmid, char __user *, shmaddr, int, shmflg) > unsigned long ret; > long err; > > + shmaddr = untagged_addr(shmaddr); > > To instead have the shmaddr set to the untagged_addr() before calling > the system call, and passing the untagged addr to the system call, as > that goes through the arm64 architecture specific code first. Indeed. For example, we already have a SYSCALL_DEFINE6(mmap, ...) in arch/arm64/kernel/sys.c, just add the untagging there. We could do something similar for the other syscalls. I don't mind doing this in the generic code but if it's only needed for arm64, I'd rather keep the generic changes to a minimum. (I had a hack overriding __SC_CAST to do this automatically for pointer arguments but this wouldn't work on mmap() and friends as the argument is unsigned long)
On Fri, Mar 29, 2019 at 11:30 AM Catalin Marinas <catalin.marinas@arm.com> wrote: > > (I trimmed down the cc list a bit since it's always bouncing) > > On Thu, Mar 28, 2019 at 02:19:34PM -0400, Steven Rostedt wrote: > > On Thu, 28 Mar 2019 19:10:07 +0100 > > Andrey Konovalov <andreyknvl@google.com> wrote: > > > > > > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> > > > > > --- > > > > > ipc/shm.c | 2 ++ > > > > > mm/madvise.c | 2 ++ > > > > > mm/mempolicy.c | 5 +++++ > > > > > mm/migrate.c | 1 + > > > > > mm/mincore.c | 2 ++ > > > > > mm/mlock.c | 5 +++++ > > > > > mm/mmap.c | 7 +++++++ > > > > > mm/mprotect.c | 1 + > > > > > mm/mremap.c | 2 ++ > > > > > mm/msync.c | 2 ++ > > > > > 10 files changed, 29 insertions(+) > > > > > > > > I wonder whether it's better to keep these as wrappers in the arm64 > > > > code. > > > > > > I don't think I understand what you propose, could you elaborate? > > > > I believe Catalin is saying that instead of placing things like: > > > > @@ -1593,6 +1593,7 @@ SYSCALL_DEFINE3(shmat, int, shmid, char __user *, shmaddr, int, shmflg) > > unsigned long ret; > > long err; > > > > + shmaddr = untagged_addr(shmaddr); > > > > To instead have the shmaddr set to the untagged_addr() before calling > > the system call, and passing the untagged addr to the system call, as > > that goes through the arm64 architecture specific code first. > > Indeed. For example, we already have a SYSCALL_DEFINE6(mmap, ...) in > arch/arm64/kernel/sys.c, just add the untagging there. We could do > something similar for the other syscalls. I don't mind doing this in the > generic code but if it's only needed for arm64, I'd rather keep the > generic changes to a minimum. Do I understand correctly, that I'll need to add ksys_ wrappers for each of the memory syscalls, and then redefine them in arch/arm64/kernel/sys.c with arm64_ prefix, like it is done for the personality syscall right now? This will require generic changes as well. > > (I had a hack overriding __SC_CAST to do this automatically for pointer > arguments but this wouldn't work on mmap() and friends as the argument > is unsigned long) > > -- > Catalin
On Tue, Apr 2, 2019 at 2:47 PM Andrey Konovalov <andreyknvl@google.com> wrote: > > On Fri, Mar 29, 2019 at 11:30 AM Catalin Marinas > <catalin.marinas@arm.com> wrote: > > > > (I trimmed down the cc list a bit since it's always bouncing) > > > > On Thu, Mar 28, 2019 at 02:19:34PM -0400, Steven Rostedt wrote: > > > On Thu, 28 Mar 2019 19:10:07 +0100 > > > Andrey Konovalov <andreyknvl@google.com> wrote: > > > > > > > > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> > > > > > > --- > > > > > > ipc/shm.c | 2 ++ > > > > > > mm/madvise.c | 2 ++ > > > > > > mm/mempolicy.c | 5 +++++ > > > > > > mm/migrate.c | 1 + > > > > > > mm/mincore.c | 2 ++ > > > > > > mm/mlock.c | 5 +++++ > > > > > > mm/mmap.c | 7 +++++++ > > > > > > mm/mprotect.c | 1 + > > > > > > mm/mremap.c | 2 ++ > > > > > > mm/msync.c | 2 ++ > > > > > > 10 files changed, 29 insertions(+) > > > > > > > > > > I wonder whether it's better to keep these as wrappers in the arm64 > > > > > code. > > > > > > > > I don't think I understand what you propose, could you elaborate? > > > > > > I believe Catalin is saying that instead of placing things like: > > > > > > @@ -1593,6 +1593,7 @@ SYSCALL_DEFINE3(shmat, int, shmid, char __user *, shmaddr, int, shmflg) > > > unsigned long ret; > > > long err; > > > > > > + shmaddr = untagged_addr(shmaddr); > > > > > > To instead have the shmaddr set to the untagged_addr() before calling > > > the system call, and passing the untagged addr to the system call, as > > > that goes through the arm64 architecture specific code first. > > > > Indeed. For example, we already have a SYSCALL_DEFINE6(mmap, ...) in > > arch/arm64/kernel/sys.c, just add the untagging there. We could do > > something similar for the other syscalls. I don't mind doing this in the > > generic code but if it's only needed for arm64, I'd rather keep the > > generic changes to a minimum. > > Do I understand correctly, that I'll need to add ksys_ wrappers for > each of the memory syscalls, and then redefine them in > arch/arm64/kernel/sys.c with arm64_ prefix, like it is done for the > personality syscall right now? This will require generic changes as > well. ping > > > > > (I had a hack overriding __SC_CAST to do this automatically for pointer > > arguments but this wouldn't work on mmap() and friends as the argument > > is unsigned long) > > > > -- > > Catalin
On Tue, Apr 02, 2019 at 02:47:34PM +0200, Andrey Konovalov wrote: > On Fri, Mar 29, 2019 at 11:30 AM Catalin Marinas > <catalin.marinas@arm.com> wrote: > > On Thu, Mar 28, 2019 at 02:19:34PM -0400, Steven Rostedt wrote: > > > On Thu, 28 Mar 2019 19:10:07 +0100 > > > Andrey Konovalov <andreyknvl@google.com> wrote: > > > > > > > > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> > > > > > > --- > > > > > > ipc/shm.c | 2 ++ > > > > > > mm/madvise.c | 2 ++ > > > > > > mm/mempolicy.c | 5 +++++ > > > > > > mm/migrate.c | 1 + > > > > > > mm/mincore.c | 2 ++ > > > > > > mm/mlock.c | 5 +++++ > > > > > > mm/mmap.c | 7 +++++++ > > > > > > mm/mprotect.c | 1 + > > > > > > mm/mremap.c | 2 ++ > > > > > > mm/msync.c | 2 ++ > > > > > > 10 files changed, 29 insertions(+) > > > > > > > > > > I wonder whether it's better to keep these as wrappers in the arm64 > > > > > code. > > > > > > > > I don't think I understand what you propose, could you elaborate? > > > > > > I believe Catalin is saying that instead of placing things like: > > > > > > @@ -1593,6 +1593,7 @@ SYSCALL_DEFINE3(shmat, int, shmid, char __user *, shmaddr, int, shmflg) > > > unsigned long ret; > > > long err; > > > > > > + shmaddr = untagged_addr(shmaddr); > > > > > > To instead have the shmaddr set to the untagged_addr() before calling > > > the system call, and passing the untagged addr to the system call, as > > > that goes through the arm64 architecture specific code first. > > > > Indeed. For example, we already have a SYSCALL_DEFINE6(mmap, ...) in > > arch/arm64/kernel/sys.c, just add the untagging there. We could do > > something similar for the other syscalls. I don't mind doing this in the > > generic code but if it's only needed for arm64, I'd rather keep the > > generic changes to a minimum. > > Do I understand correctly, that I'll need to add ksys_ wrappers for > each of the memory syscalls, and then redefine them in > arch/arm64/kernel/sys.c with arm64_ prefix, like it is done for the > personality syscall right now? This will require generic changes as > well. Yes. My aim is to keep the number of untagged_addr() calls in the generic code to a minimum (rather than just keeping the generic code changes small).
On Fri, Apr 26, 2019 at 4:17 PM Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Tue, Apr 02, 2019 at 02:47:34PM +0200, Andrey Konovalov wrote: > > On Fri, Mar 29, 2019 at 11:30 AM Catalin Marinas > > <catalin.marinas@arm.com> wrote: > > > On Thu, Mar 28, 2019 at 02:19:34PM -0400, Steven Rostedt wrote: > > > > On Thu, 28 Mar 2019 19:10:07 +0100 > > > > Andrey Konovalov <andreyknvl@google.com> wrote: > > > > > > > > > > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> > > > > > > > --- > > > > > > > ipc/shm.c | 2 ++ > > > > > > > mm/madvise.c | 2 ++ > > > > > > > mm/mempolicy.c | 5 +++++ > > > > > > > mm/migrate.c | 1 + > > > > > > > mm/mincore.c | 2 ++ > > > > > > > mm/mlock.c | 5 +++++ > > > > > > > mm/mmap.c | 7 +++++++ > > > > > > > mm/mprotect.c | 1 + > > > > > > > mm/mremap.c | 2 ++ > > > > > > > mm/msync.c | 2 ++ > > > > > > > 10 files changed, 29 insertions(+) > > > > > > > > > > > > I wonder whether it's better to keep these as wrappers in the arm64 > > > > > > code. > > > > > > > > > > I don't think I understand what you propose, could you elaborate? > > > > > > > > I believe Catalin is saying that instead of placing things like: > > > > > > > > @@ -1593,6 +1593,7 @@ SYSCALL_DEFINE3(shmat, int, shmid, char __user *, shmaddr, int, shmflg) > > > > unsigned long ret; > > > > long err; > > > > > > > > + shmaddr = untagged_addr(shmaddr); > > > > > > > > To instead have the shmaddr set to the untagged_addr() before calling > > > > the system call, and passing the untagged addr to the system call, as > > > > that goes through the arm64 architecture specific code first. > > > > > > Indeed. For example, we already have a SYSCALL_DEFINE6(mmap, ...) in > > > arch/arm64/kernel/sys.c, just add the untagging there. We could do > > > something similar for the other syscalls. I don't mind doing this in the > > > generic code but if it's only needed for arm64, I'd rather keep the > > > generic changes to a minimum. > > > > Do I understand correctly, that I'll need to add ksys_ wrappers for > > each of the memory syscalls, and then redefine them in > > arch/arm64/kernel/sys.c with arm64_ prefix, like it is done for the > > personality syscall right now? This will require generic changes as > > well. > > Yes. My aim is to keep the number of untagged_addr() calls in the > generic code to a minimum (rather than just keeping the generic code > changes small). OK, will do in v14 (despite it still being unclear whether we should do untagging here or not). > > -- > Catalin
diff --git a/ipc/shm.c b/ipc/shm.c index ce1ca9f7c6e9..7af8951e6c41 100644 --- a/ipc/shm.c +++ b/ipc/shm.c @@ -1593,6 +1593,7 @@ SYSCALL_DEFINE3(shmat, int, shmid, char __user *, shmaddr, int, shmflg) unsigned long ret; long err; + shmaddr = untagged_addr(shmaddr); err = do_shmat(shmid, shmaddr, shmflg, &ret, SHMLBA); if (err) return err; @@ -1732,6 +1733,7 @@ long ksys_shmdt(char __user *shmaddr) SYSCALL_DEFINE1(shmdt, char __user *, shmaddr) { + shmaddr = untagged_addr(shmaddr); return ksys_shmdt(shmaddr); } diff --git a/mm/madvise.c b/mm/madvise.c index 21a7881a2db4..64e6d34a7f9b 100644 --- a/mm/madvise.c +++ b/mm/madvise.c @@ -809,6 +809,8 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior) size_t len; struct blk_plug plug; + start = untagged_addr(start); + if (!madvise_behavior_valid(behavior)) return error; diff --git a/mm/mempolicy.c b/mm/mempolicy.c index af171ccb56a2..31691737c59c 100644 --- a/mm/mempolicy.c +++ b/mm/mempolicy.c @@ -1334,6 +1334,7 @@ static long kernel_mbind(unsigned long start, unsigned long len, int err; unsigned short mode_flags; + start = untagged_addr(start); mode_flags = mode & MPOL_MODE_FLAGS; mode &= ~MPOL_MODE_FLAGS; if (mode >= MPOL_MAX) @@ -1491,6 +1492,8 @@ static int kernel_get_mempolicy(int __user *policy, int uninitialized_var(pval); nodemask_t nodes; + addr = untagged_addr(addr); + if (nmask != NULL && maxnode < nr_node_ids) return -EINVAL; @@ -1576,6 +1579,8 @@ COMPAT_SYSCALL_DEFINE6(mbind, compat_ulong_t, start, compat_ulong_t, len, unsigned long nr_bits, alloc_size; nodemask_t bm; + start = untagged_addr(start); + nr_bits = min_t(unsigned long, maxnode-1, MAX_NUMNODES); alloc_size = ALIGN(nr_bits, BITS_PER_LONG) / 8; diff --git a/mm/migrate.c b/mm/migrate.c index ac6f4939bb59..ecc6dcdefb1f 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -1612,6 +1612,7 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, if (get_user(node, nodes + i)) goto out_flush; addr = (unsigned long)p; + addr = untagged_addr(addr); err = -ENODEV; if (node < 0 || node >= MAX_NUMNODES) diff --git a/mm/mincore.c b/mm/mincore.c index 218099b5ed31..c4a3f4484b6b 100644 --- a/mm/mincore.c +++ b/mm/mincore.c @@ -228,6 +228,8 @@ SYSCALL_DEFINE3(mincore, unsigned long, start, size_t, len, unsigned long pages; unsigned char *tmp; + start = untagged_addr(start); + /* Check the start address: needs to be page-aligned.. */ if (start & ~PAGE_MASK) return -EINVAL; diff --git a/mm/mlock.c b/mm/mlock.c index 080f3b36415b..6934ec92bf39 100644 --- a/mm/mlock.c +++ b/mm/mlock.c @@ -715,6 +715,7 @@ static __must_check int do_mlock(unsigned long start, size_t len, vm_flags_t fla SYSCALL_DEFINE2(mlock, unsigned long, start, size_t, len) { + start = untagged_addr(start); return do_mlock(start, len, VM_LOCKED); } @@ -722,6 +723,8 @@ SYSCALL_DEFINE3(mlock2, unsigned long, start, size_t, len, int, flags) { vm_flags_t vm_flags = VM_LOCKED; + start = untagged_addr(start); + if (flags & ~MLOCK_ONFAULT) return -EINVAL; @@ -735,6 +738,8 @@ SYSCALL_DEFINE2(munlock, unsigned long, start, size_t, len) { int ret; + start = untagged_addr(start); + len = PAGE_ALIGN(len + (offset_in_page(start))); start &= PAGE_MASK; diff --git a/mm/mmap.c b/mm/mmap.c index 41eb48d9b527..512c679c7f33 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -199,6 +199,8 @@ SYSCALL_DEFINE1(brk, unsigned long, brk) bool downgraded = false; LIST_HEAD(uf); + brk = untagged_addr(brk); + if (down_write_killable(&mm->mmap_sem)) return -EINTR; @@ -1571,6 +1573,8 @@ unsigned long ksys_mmap_pgoff(unsigned long addr, unsigned long len, struct file *file = NULL; unsigned long retval; + addr = untagged_addr(addr); + if (!(flags & MAP_ANONYMOUS)) { audit_mmap_fd(fd, flags); file = fget(fd); @@ -2867,6 +2871,7 @@ EXPORT_SYMBOL(vm_munmap); SYSCALL_DEFINE2(munmap, unsigned long, addr, size_t, len) { + addr = untagged_addr(addr); profile_munmap(addr); return __vm_munmap(addr, len, true); } @@ -2885,6 +2890,8 @@ SYSCALL_DEFINE5(remap_file_pages, unsigned long, start, unsigned long, size, unsigned long ret = -EINVAL; struct file *file; + start = untagged_addr(start); + pr_warn_once("%s (%d) uses deprecated remap_file_pages() syscall. See Documentation/vm/remap_file_pages.rst.\n", current->comm, current->pid); diff --git a/mm/mprotect.c b/mm/mprotect.c index 028c724dcb1a..3c2b11629f89 100644 --- a/mm/mprotect.c +++ b/mm/mprotect.c @@ -468,6 +468,7 @@ static int do_mprotect_pkey(unsigned long start, size_t len, if (grows == (PROT_GROWSDOWN|PROT_GROWSUP)) /* can't be both */ return -EINVAL; + start = untagged_addr(start); if (start & ~PAGE_MASK) return -EINVAL; if (!len) diff --git a/mm/mremap.c b/mm/mremap.c index e3edef6b7a12..6422aeee65bb 100644 --- a/mm/mremap.c +++ b/mm/mremap.c @@ -605,6 +605,8 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len, LIST_HEAD(uf_unmap_early); LIST_HEAD(uf_unmap); + addr = untagged_addr(addr); + if (flags & ~(MREMAP_FIXED | MREMAP_MAYMOVE)) return ret; diff --git a/mm/msync.c b/mm/msync.c index ef30a429623a..c3bd3e75f687 100644 --- a/mm/msync.c +++ b/mm/msync.c @@ -37,6 +37,8 @@ SYSCALL_DEFINE3(msync, unsigned long, start, size_t, len, int, flags) int unmapped_error = 0; int error = -EINVAL; + start = untagged_addr(start); + if (flags & ~(MS_ASYNC | MS_INVALIDATE | MS_SYNC)) goto out; if (offset_in_page(start))
This patch is a part of a series that extends arm64 kernel ABI to allow to pass tagged user pointers (with the top byte set to something else other than 0x00) as syscall arguments. This patch allows tagged pointers to be passed to the following memory syscalls: madvise, mbind, get_mempolicy, mincore, mlock, mlock2, brk, mmap_pgoff, old_mmap, munmap, remap_file_pages, mprotect, pkey_mprotect, mremap, msync and shmdt. This is done by untagging pointers passed to these syscalls in the prologues of their handlers. Signed-off-by: Andrey Konovalov <andreyknvl@google.com> --- ipc/shm.c | 2 ++ mm/madvise.c | 2 ++ mm/mempolicy.c | 5 +++++ mm/migrate.c | 1 + mm/mincore.c | 2 ++ mm/mlock.c | 5 +++++ mm/mmap.c | 7 +++++++ mm/mprotect.c | 1 + mm/mremap.c | 2 ++ mm/msync.c | 2 ++ 10 files changed, 29 insertions(+)