From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Garnier Subject: Re: [PATCH v2 4/4] arm64/syscalls: Specific usage of verify_pre_usermode_state Date: Thu, 9 Mar 2017 08:19:31 -0800 Message-ID: References: <20170309012456.5631-1-thgarnie@google.com> <20170309012456.5631-4-thgarnie@google.com> <20170309122354.GB6320@leverpostej> <20170309160551.GC11966@leverpostej> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: List-Post: List-Help: List-Unsubscribe: List-Subscribe: In-Reply-To: <20170309160551.GC11966@leverpostej> To: Mark Rutland Cc: David Howells , Dave Hansen , Arnd Bergmann , Al Viro , =?UTF-8?Q?Ren=C3=A9_Nyffenegger?= , Andrew Morton , Kees Cook , "Paul E . McKenney" , "David S . Miller" , Andy Lutomirski , Ard Biesheuvel , Nicolas Pitre , Petr Mladek , Sebastian Andrzej Siewior , Sergey Senozhatsky , Helge Deller , Rik van Riel , Ingo Molnar , Oleg Nesterov , John Stultz , Thomas Gleixner , Pavel Tikhomirov List-Id: linux-api@vger.kernel.org On Thu, Mar 9, 2017 at 8:05 AM, Mark Rutland wrote: > On Thu, Mar 09, 2017 at 07:56:49AM -0800, Thomas Garnier wrote: >> On Thu, Mar 9, 2017 at 4:23 AM, Mark Rutland wrote: >> > We generally stick to lower case for the arm64 assembly macros. If we >> > need this, we should stick to the existing convention. >> > >> >> +/* Similar to set_fs(USER_DS) in verify_pre_usermode_state without a warning. */ >> >> +.macro VERIFY_PRE_USERMODE_STATE >> >> + mov x1, #TASK_SIZE_64 >> >> + str x1, [tsk, #TSK_TI_ADDR_LIMIT] >> >> +.endm >> > >> > We need arm64's set_fs() to configure UAO, too, so this is much weaker >> > than set_fs(), and will leave __{get,put}_user and >> > __copy_{to,from}_user() able to access kernel memory. >> > >> > We don't currently have an asm helper to clear UAO, and unconditionally >> > poking that on exception return is liable to be somewhat expensive. >> > >> > Also, given we're only trying to catch this in syscalls, I'm afraid I >> > don't see what we gain by doing this in the entry assembly. >> >> I optimized all architectures from the arm (32-bit) discussion. I will >> come back to a simple bl to the verify function. Thanks! > > What I was trying to ask was do we need to touch the assembly at all > here? You don't but he generic solution add code to every single syscall. > Are we trying to protect the non-syscall cases by doing this in > assembly? If so, it'd be worth calling out in the commit message. It is an added benefit but not required. > If so, we could add the necessary helper to clear UAO. I can look at set_fs and fix it on the next iteraiton. > If not, doing this in the entry assembly only saves the small overhead > of reading and comparing the addr_limit for in-kernel use of the > syscalls (e.g. in the compat wrappers), and we may as well rely on the > common !ARCH_NO_SYSCALL_VERIFY_PRE_USERMODE_STATE implementation. You also don't have the code added for each syscall and a call. > > Thanks, > Mark. -- Thomas From mboxrd@z Thu Jan 1 00:00:00 1970 From: thgarnie@google.com (Thomas Garnier) Date: Thu, 9 Mar 2017 08:19:31 -0800 Subject: [PATCH v2 4/4] arm64/syscalls: Specific usage of verify_pre_usermode_state In-Reply-To: <20170309160551.GC11966@leverpostej> References: <20170309012456.5631-1-thgarnie@google.com> <20170309012456.5631-4-thgarnie@google.com> <20170309122354.GB6320@leverpostej> <20170309160551.GC11966@leverpostej> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Thu, Mar 9, 2017 at 8:05 AM, Mark Rutland wrote: > On Thu, Mar 09, 2017 at 07:56:49AM -0800, Thomas Garnier wrote: >> On Thu, Mar 9, 2017 at 4:23 AM, Mark Rutland wrote: >> > We generally stick to lower case for the arm64 assembly macros. If we >> > need this, we should stick to the existing convention. >> > >> >> +/* Similar to set_fs(USER_DS) in verify_pre_usermode_state without a warning. */ >> >> +.macro VERIFY_PRE_USERMODE_STATE >> >> + mov x1, #TASK_SIZE_64 >> >> + str x1, [tsk, #TSK_TI_ADDR_LIMIT] >> >> +.endm >> > >> > We need arm64's set_fs() to configure UAO, too, so this is much weaker >> > than set_fs(), and will leave __{get,put}_user and >> > __copy_{to,from}_user() able to access kernel memory. >> > >> > We don't currently have an asm helper to clear UAO, and unconditionally >> > poking that on exception return is liable to be somewhat expensive. >> > >> > Also, given we're only trying to catch this in syscalls, I'm afraid I >> > don't see what we gain by doing this in the entry assembly. >> >> I optimized all architectures from the arm (32-bit) discussion. I will >> come back to a simple bl to the verify function. Thanks! > > What I was trying to ask was do we need to touch the assembly at all > here? You don't but he generic solution add code to every single syscall. > Are we trying to protect the non-syscall cases by doing this in > assembly? If so, it'd be worth calling out in the commit message. It is an added benefit but not required. > If so, we could add the necessary helper to clear UAO. I can look at set_fs and fix it on the next iteraiton. > If not, doing this in the entry assembly only saves the small overhead > of reading and comparing the addr_limit for in-kernel use of the > syscalls (e.g. in the compat wrappers), and we may as well rely on the > common !ARCH_NO_SYSCALL_VERIFY_PRE_USERMODE_STATE implementation. You also don't have the code added for each syscall and a call. > > Thanks, > Mark. -- Thomas From mboxrd@z Thu Jan 1 00:00:00 1970 MIME-Version: 1.0 In-Reply-To: <20170309160551.GC11966@leverpostej> References: <20170309012456.5631-1-thgarnie@google.com> <20170309012456.5631-4-thgarnie@google.com> <20170309122354.GB6320@leverpostej> <20170309160551.GC11966@leverpostej> From: Thomas Garnier Date: Thu, 9 Mar 2017 08:19:31 -0800 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: [kernel-hardening] Re: [PATCH v2 4/4] arm64/syscalls: Specific usage of verify_pre_usermode_state To: Mark Rutland Cc: David Howells , Dave Hansen , Arnd Bergmann , Al Viro , =?UTF-8?Q?Ren=C3=A9_Nyffenegger?= , Andrew Morton , Kees Cook , "Paul E . McKenney" , "David S . Miller" , Andy Lutomirski , Ard Biesheuvel , Nicolas Pitre , Petr Mladek , Sebastian Andrzej Siewior , Sergey Senozhatsky , Helge Deller , Rik van Riel , Ingo Molnar , Oleg Nesterov , John Stultz , Thomas Gleixner , Pavel Tikhomirov , Frederic Weisbecker , Stephen Smalley , Stanislav Kinsburskiy , Ingo Molnar , "H . Peter Anvin" , Paolo Bonzini , Borislav Petkov , Josh Poimboeuf , Brian Gerst , Jan Beulich , Christian Borntraeger , "Luis R . Rodriguez" , He Chen , Russell King , Will Deacon , Catalin Marinas , James Morse , Pratyush Anand , Vladimir Murzin , Chris Metcalf , Andre Przywara , Linux API , LKML , the arch/x86 maintainers , linux-arm-kernel@lists.infradead.org, Kernel Hardening List-ID: On Thu, Mar 9, 2017 at 8:05 AM, Mark Rutland wrote: > On Thu, Mar 09, 2017 at 07:56:49AM -0800, Thomas Garnier wrote: >> On Thu, Mar 9, 2017 at 4:23 AM, Mark Rutland wrote: >> > We generally stick to lower case for the arm64 assembly macros. If we >> > need this, we should stick to the existing convention. >> > >> >> +/* Similar to set_fs(USER_DS) in verify_pre_usermode_state without a warning. */ >> >> +.macro VERIFY_PRE_USERMODE_STATE >> >> + mov x1, #TASK_SIZE_64 >> >> + str x1, [tsk, #TSK_TI_ADDR_LIMIT] >> >> +.endm >> > >> > We need arm64's set_fs() to configure UAO, too, so this is much weaker >> > than set_fs(), and will leave __{get,put}_user and >> > __copy_{to,from}_user() able to access kernel memory. >> > >> > We don't currently have an asm helper to clear UAO, and unconditionally >> > poking that on exception return is liable to be somewhat expensive. >> > >> > Also, given we're only trying to catch this in syscalls, I'm afraid I >> > don't see what we gain by doing this in the entry assembly. >> >> I optimized all architectures from the arm (32-bit) discussion. I will >> come back to a simple bl to the verify function. Thanks! > > What I was trying to ask was do we need to touch the assembly at all > here? You don't but he generic solution add code to every single syscall. > Are we trying to protect the non-syscall cases by doing this in > assembly? If so, it'd be worth calling out in the commit message. It is an added benefit but not required. > If so, we could add the necessary helper to clear UAO. I can look at set_fs and fix it on the next iteraiton. > If not, doing this in the entry assembly only saves the small overhead > of reading and comparing the addr_limit for in-kernel use of the > syscalls (e.g. in the compat wrappers), and we may as well rely on the > common !ARCH_NO_SYSCALL_VERIFY_PRE_USERMODE_STATE implementation. You also don't have the code added for each syscall and a call. > > Thanks, > Mark. -- Thomas