Re: [PATCH v2 4/4] arm64/syscalls: Specific usage of verify_pre_usermode_state

On Thu, Mar 9, 2017 at 8:05 AM, Mark Rutland [off-list ref] wrote:

On Thu, Mar 09, 2017 at 07:56:49AM -0800, Thomas Garnier wrote:

quoted

On Thu, Mar 9, 2017 at 4:23 AM, Mark Rutland [off-list ref] wrote:

quoted

We generally stick to lower case for the arm64 assembly macros. If we
need this, we should stick to the existing convention.

quoted

+/* 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