From mboxrd@z Thu Jan 1 00:00:00 1970 From: Catalin Marinas Subject: Re: [PATCH v16 02/16] arm64: untag user pointers in access_ok and __uaccess_mask_ptr Date: Wed, 12 Jun 2019 10:32:00 +0100 Message-ID: <20190612093158.GG10165@c02tf0j2hf1t.cambridge.arm.com> References: <4327b260fb17c4776a1e3c844f388e4948cfb747.1559580831.git.andreyknvl@google.com> <20190610175326.GC25803@arrakis.emea.arm.com> <20190611145720.GA63588@arrakis.emea.arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Vincenzo Frascino Cc: Andrey Konovalov , Mark Rutland , kvm@vger.kernel.org, Szabolcs Nagy , Will Deacon , dri-devel@lists.freedesktop.org, linux-mm@kvack.org, Khalid Aziz , linux-kselftest@vger.kernel.org, Felix Kuehling , Jacob Bramley , Leon Romanovsky , linux-rdma@vger.kernel.org, amd-gfx@lists.freedesktop.org, Christoph Hellwig , Jason Gunthorpe , Dmitry Vyukov , Dave Martin , Evgeniy Stepanov , linux-media@vger.kernel.org, Kevin Brodsky , Kees Cook List-Id: linux-rdma@vger.kernel.org Hi Vincenzo, On Tue, Jun 11, 2019 at 06:09:10PM +0100, Vincenzo Frascino wrote: > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > > index 3767fb21a5b8..69d0be1fc708 100644 > > --- a/arch/arm64/kernel/process.c > > +++ b/arch/arm64/kernel/process.c > > @@ -30,6 +30,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -323,6 +324,7 @@ void flush_thread(void) > > fpsimd_flush_thread(); > > tls_thread_flush(); > > flush_ptrace_hw_breakpoint(current); > > + clear_thread_flag(TIF_TAGGED_ADDR); > > Nit: in line we the other functions in thread_flush we could have something like > "tagged_addr_thread_flush", maybe inlined. The other functions do a lot more than clearing a TIF flag, so they deserved their own place. We could do this when adding MTE support. I think we also need to check what other TIF flags we may inadvertently pass on execve(), maybe have a mask clearing. > > diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h > > index 094bb03b9cc2..2e927b3e9d6c 100644 > > --- a/include/uapi/linux/prctl.h > > +++ b/include/uapi/linux/prctl.h > > @@ -229,4 +229,9 @@ struct prctl_mm_map { > > # define PR_PAC_APDBKEY (1UL << 3) > > # define PR_PAC_APGAKEY (1UL << 4) > > > > +/* Tagged user address controls for arm64 */ > > +#define PR_SET_TAGGED_ADDR_CTRL 55 > > +#define PR_GET_TAGGED_ADDR_CTRL 56 > > +# define PR_TAGGED_ADDR_ENABLE (1UL << 0) > > + > > #endif /* _LINUX_PRCTL_H */ > > diff --git a/kernel/sys.c b/kernel/sys.c > > index 2969304c29fe..ec48396b4943 100644 > > --- a/kernel/sys.c > > +++ b/kernel/sys.c > > @@ -124,6 +124,12 @@ > > #ifndef PAC_RESET_KEYS > > # define PAC_RESET_KEYS(a, b) (-EINVAL) > > #endif > > +#ifndef SET_TAGGED_ADDR_CTRL > > +# define SET_TAGGED_ADDR_CTRL(a) (-EINVAL) > > +#endif > > +#ifndef GET_TAGGED_ADDR_CTRL > > +# define GET_TAGGED_ADDR_CTRL() (-EINVAL) > > +#endif > > > > /* > > * this is where the system-wide overflow UID and GID are defined, for > > @@ -2492,6 +2498,16 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, > > return -EINVAL; > > error = PAC_RESET_KEYS(me, arg2); > > break; > > + case PR_SET_TAGGED_ADDR_CTRL: > > + if (arg3 || arg4 || arg5) > > + return -EINVAL; > > + error = SET_TAGGED_ADDR_CTRL(arg2); > > + break; > > + case PR_GET_TAGGED_ADDR_CTRL: > > + if (arg2 || arg3 || arg4 || arg5) > > + return -EINVAL; > > + error = GET_TAGGED_ADDR_CTRL(); > > + break; > > Why do we need two prctl here? We could have only one and use arg2 as set/get > and arg3 as a parameter. What do you think? This follows the other PR_* options, e.g. PR_SET_VL/GET_VL, PR_*_FP_MODE. We will use other bits in arg2, for example to set the precise vs imprecise MTE trapping. -- Catalin