| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 22 May 2019 12:49:10 +0100
From: Catalin Marinas <catalin.marinas@....com>
To: Andrey Konovalov <andreyknvl@...gle.com>
Cc: linux-arm-kernel@...ts.infradead.org, linux-mm@...ck.org,
linux-kernel@...r.kernel.org, amd-gfx@...ts.freedesktop.org,
dri-devel@...ts.freedesktop.org, linux-rdma@...r.kernel.org,
linux-media@...r.kernel.org, kvm@...r.kernel.org,
linux-kselftest@...r.kernel.org,
Vincenzo Frascino <vincenzo.frascino@....com>,
Will Deacon <will.deacon@....com>,
Mark Rutland <mark.rutland@....com>,
Andrew Morton <akpm@...ux-foundation.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Kees Cook <keescook@...omium.org>,
Yishai Hadas <yishaih@...lanox.com>,
Felix Kuehling <Felix.Kuehling@....com>,
Alexander Deucher <Alexander.Deucher@....com>,
Christian Koenig <Christian.Koenig@....com>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Jens Wiklander <jens.wiklander@...aro.org>,
Alex Williamson <alex.williamson@...hat.com>,
Leon Romanovsky <leon@...nel.org>,
Dmitry Vyukov <dvyukov@...gle.com>,
Kostya Serebryany <kcc@...gle.com>,
Evgeniy Stepanov <eugenis@...gle.com>,
Lee Smith <Lee.Smith@....com>,
Ramana Radhakrishnan <Ramana.Radhakrishnan@....com>,
Jacob Bramley <Jacob.Bramley@....com>,
Ruben Ayrapetyan <Ruben.Ayrapetyan@....com>,
Robin Murphy <robin.murphy@....com>,
Luc Van Oostenryck <luc.vanoostenryck@...il.com>,
Dave Martin <Dave.Martin@....com>,
Kevin Brodsky <kevin.brodsky@....com>,
Szabolcs Nagy <Szabolcs.Nagy@....com>
Subject: Re: [PATCH v15 05/17] arms64: untag user pointers passed to memory
syscalls
On Mon, May 06, 2019 at 06:30:51PM +0200, 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: brk, get_mempolicy, madvise, mbind, mincore, mlock, mlock2,
> mmap, mmap_pgoff, mprotect, mremap, msync, munlock, munmap,
> remap_file_pages, shmat and shmdt.
>
> This is done by untagging pointers passed to these syscalls in the
> prologues of their handlers.
I'll go through them one by one to see if we can tighten the expected
ABI while having the MTE in mind.
> diff --git a/arch/arm64/kernel/sys.c b/arch/arm64/kernel/sys.c
> index b44065fb1616..933bb9f3d6ec 100644
> --- a/arch/arm64/kernel/sys.c
> +++ b/arch/arm64/kernel/sys.c
> @@ -35,10 +35,33 @@ SYSCALL_DEFINE6(mmap, unsigned long, addr, unsigned long, len,
> {
> if (offset_in_page(off) != 0)
> return -EINVAL;
> -
> + addr = untagged_addr(addr);
> return ksys_mmap_pgoff(addr, len, prot, flags, fd, off >> PAGE_SHIFT);
> }
If user passes a tagged pointer to mmap() and the address is honoured
(or MAP_FIXED is given), what is the expected return pointer? Does it
need to be tagged with the value from the hint?
With MTE, we may want to use this as a request for the default colour of
the mapped pages (still under discussion).
> +SYSCALL_DEFINE6(arm64_mmap_pgoff, unsigned long, addr, unsigned long, len,
> + unsigned long, prot, unsigned long, flags,
> + unsigned long, fd, unsigned long, pgoff)
> +{
> + addr = untagged_addr(addr);
> + return ksys_mmap_pgoff(addr, len, prot, flags, fd, pgoff);
> +}
We don't have __NR_mmap_pgoff on arm64.
> +SYSCALL_DEFINE5(arm64_mremap, unsigned long, addr, unsigned long, old_len,
> + unsigned long, new_len, unsigned long, flags,
> + unsigned long, new_addr)
> +{
> + addr = untagged_addr(addr);
> + new_addr = untagged_addr(new_addr);
> + return ksys_mremap(addr, old_len, new_len, flags, new_addr);
> +}
Similar comment as for mmap(), do we want the tag from new_addr to be
preserved? In addition, should we check that the two tags are identical
or mremap() should become a way to repaint a memory region?
> +SYSCALL_DEFINE2(arm64_munmap, unsigned long, addr, size_t, len)
> +{
> + addr = untagged_addr(addr);
> + return ksys_munmap(addr, len);
> +}
This looks fine.
> +SYSCALL_DEFINE1(arm64_brk, unsigned long, brk)
> +{
> + brk = untagged_addr(brk);
> + return ksys_brk(brk);
> +}
I wonder whether brk() should simply not accept tags, and should not
return them (similar to the prctl(PR_SET_MM) discussion). We could
document this in the ABI requirements.
> +SYSCALL_DEFINE5(arm64_get_mempolicy, int __user *, policy,
> + unsigned long __user *, nmask, unsigned long, maxnode,
> + unsigned long, addr, unsigned long, flags)
> +{
> + addr = untagged_addr(addr);
> + return ksys_get_mempolicy(policy, nmask, maxnode, addr, flags);
> +}
> +
> +SYSCALL_DEFINE3(arm64_madvise, unsigned long, start,
> + size_t, len_in, int, behavior)
> +{
> + start = untagged_addr(start);
> + return ksys_madvise(start, len_in, behavior);
> +}
> +
> +SYSCALL_DEFINE6(arm64_mbind, unsigned long, start, unsigned long, len,
> + unsigned long, mode, const unsigned long __user *, nmask,
> + unsigned long, maxnode, unsigned int, flags)
> +{
> + start = untagged_addr(start);
> + return ksys_mbind(start, len, mode, nmask, maxnode, flags);
> +}
> +
> +SYSCALL_DEFINE2(arm64_mlock, unsigned long, start, size_t, len)
> +{
> + start = untagged_addr(start);
> + return ksys_mlock(start, len, VM_LOCKED);
> +}
> +
> +SYSCALL_DEFINE2(arm64_mlock2, unsigned long, start, size_t, len)
> +{
> + start = untagged_addr(start);
> + return ksys_mlock(start, len, VM_LOCKED);
> +}
> +
> +SYSCALL_DEFINE2(arm64_munlock, unsigned long, start, size_t, len)
> +{
> + start = untagged_addr(start);
> + return ksys_munlock(start, len);
> +}
> +
> +SYSCALL_DEFINE3(arm64_mprotect, unsigned long, start, size_t, len,
> + unsigned long, prot)
> +{
> + start = untagged_addr(start);
> + return ksys_mprotect_pkey(start, len, prot, -1);
> +}
> +
> +SYSCALL_DEFINE3(arm64_msync, unsigned long, start, size_t, len, int, flags)
> +{
> + start = untagged_addr(start);
> + return ksys_msync(start, len, flags);
> +}
> +
> +SYSCALL_DEFINE3(arm64_mincore, unsigned long, start, size_t, len,
> + unsigned char __user *, vec)
> +{
> + start = untagged_addr(start);
> + return ksys_mincore(start, len, vec);
> +}
These look fine.
> +SYSCALL_DEFINE5(arm64_remap_file_pages, unsigned long, start,
> + unsigned long, size, unsigned long, prot,
> + unsigned long, pgoff, unsigned long, flags)
> +{
> + start = untagged_addr(start);
> + return ksys_remap_file_pages(start, size, prot, pgoff, flags);
> +}
While this has been deprecated for some time, I presume user space still
invokes it?
> +SYSCALL_DEFINE3(arm64_shmat, int, shmid, char __user *, shmaddr, int, shmflg)
> +{
> + shmaddr = untagged_addr(shmaddr);
> + return ksys_shmat(shmid, shmaddr, shmflg);
> +}
> +
> +SYSCALL_DEFINE1(arm64_shmdt, char __user *, shmaddr)
> +{
> + shmaddr = untagged_addr(shmaddr);
> + return ksys_shmdt(shmaddr);
> +}
Do we actually want to allow shared tagged memory? Who's going to tag
it? If not, we can document it as not supported.
--
Catalin
Powered by blists - more mailing lists