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

----- 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
[off-list ref] wrote:

quoted

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