Re: [RFC PATCH for 4.18 1/2] rseq: validate rseq_cs fields are < TASK_SIZE

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

----- On Jun 28, 2018, at 4:22 PM, Andy Lutomirski luto@kernel.org wrote:

> On Thu, Jun 28, 2018 at 9:23 AM, Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>> Validating the abort_ip field of rseq_cs ensures that the kernel don't
>> return to an invalid address when returning to userspace after an abort.
>> I don't fully trust each architecture code to cleanly deal with invalid
>> return addresses.
>>
>> Validating the range [ start_ip, start_ip + post_commit_offset ] is an
>> extra validation step ensuring that userspace provides valid values to
>> describe the critical section.
>>
>> If validation fails, the process is killed with a segmentation fault.
>>
>> Change the rseq ABI so rseq_cs start_ip, post_commit_offset and abort_ip
>> fields are seen as 64-bit fields by both 32-bit and 64-bit kernels rather
>> that ignoring the 32 upper bits on 32-bit kernels. This ensures we have
>> a consistent behavior for a 32-bit binary executed on 32-bit kernels and
>> in compat mode on 64-bit kernels.
> 
> This is okay with me for a fix outside the merge window.  Can you do a
> followup for the next merge window that fixes it better, though?  In
> particular, TASK_SIZE is generally garbage.  I think a better fix
> would be something like adding a new arch-overridable helper like:
> 
> static inline unsigned long current_max_user_addr(void) { return TASK_SIZE; }
> 
> and overriding it on x86 as something like:
> 
> static inline unsigned long current_max_user_addr(void) {
> #ifdef CONFIG_IA32_EMULATION
>  return user_64bit_mode(current_pt_regs()) ? TASK_SIZE_MAX : (1UL << 32) - 1;
> #else
>  return TASK_SIZE_MAX;
> }
> 
> TASK_SIZE really needs to die.

Sure, I'll put it in my backlog.

Thanks!

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com