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

----- On Jun 29, 2018, at 4:39 PM, Andy Lutomirski luto@amacapital.net wrote:

On Fri, Jun 29, 2018 at 12:48 PM, Mathieu Desnoyers
[off-list ref] wrote:

quoted

There are two aspects I'm concerned about here:

1) security: we don't want 32-bit user-space to feed a 64-bit value over 4GB
   as abort_ip that may end up causing OOPSes on architectures that would
   lack proper validation of those values on return to userspace.

I'm not too worried about this.  As long as you're doing it from
signal-delivery context (which you are AFAICT) you're fine.

No, it's not just signal-delivery context. It's _also_ called from
return to usermode loop, which can by called on return from
interrupt/trap/syscall.

But I re-read the code and I think I have a really straightforward
solution.  Two choices:

(1) Change instruction_pointer_set() to return an error code if the
address passed in is garbage in a way that could cause unexpected
behavior (like >=2^32 on x86_64 if regs->cs is 32-bit).  It has very
very few callers.

This would take care of my security concern wrt abort_ip, but would not
provide consistent behavior for the other fields. Also, perhaps this
kind of change should aim the next merge window ?

(2) Add instruction_pointer_validate() to go along with
instruction_pointer_set().

That should be enough to solve the problem, right?

This would only handle the "security" part of the matter, which
is specifically related to rseq->rseq_cs->abort_ip.

What is left is ensuring that we have consistent behavior for
other fields:

[ Note: we have introduced this helper macro: LINUX_FIELD_u32_u64
which defines a field which is 64-bit for 64-bit processes, and 32-bit
with 32-bit of padding for 32-bit processes. ]

* rseq->rseq_cs: (userspace pointer to user-space, updated by user-space
  with single-copy atomicity): current type: LINUX_FIELD_u32_u64,
  cannot be changed to __u64 due to single-copy atomicity requirement,

* rseq->rseq_cs->start_ip: currently a LINUX_FIELD_u32_u64,
  could become a __u64,

* rseq->rseq_cs->post_commit_ip: currently a LINUX_FIELD_u32_u64,
  could become a __u64,

* rseq->rseq_cs->abort_ip: currently a LINUX_FIELD_u32_u64,
  could become a __u64,

For abort_ip, changing the type to __u64 and using the
instruction_pointer_validate() approach you propose would work.

For start_ip and post_commit_ip, we need to decide whether we
want to kill a 32-bit process setting the high bits or if we just
accept and use the full __u64 content on both 32-bit and 64-bit
kernels. Those two fields are only used for arithmetic comparison.
Using the full __u64 content means using 64-bit arithmetic on
32-bit native kernels though.

If we decide to kill the offending process (whether the field type is
__u64 or LINUX_FIELD_u32_u64), we need to be able to figure out if
the process is a compat task when called from signal delivery and from
return to usermode loop (return from irq/trap/syscall).

Comparison with TASK_SIZE solves the issue for abort_ip, post_commit_ip
and start_ip, although nobody seems to like that approach very much.

For rseq->rseq_cs, we cannot use __u64 due to single-copy atomicity
update requirement for 32-bit processes. However, we are using this
field in a copy_from_user(), so it will EFAULT if the high-bits are
set by a compat 32-bit task on a 64-bit kernel. We can therefore check
that the padding is zeroed explicitly on a native 32-bit kernel to
provide a consistent behavior. Specifically because rseq->rseq_cs is
checked with access_ok(), it is therefore enough to check the padding
when __LP64__ is not defined by the preprocessor.

But rather than trying to play games with input validation, I would
favor an approach that would allow rseq to validate all its inputs
straightforwardly. Introducing user_64bit_mode(struct pt_regs *)
across all architectures would allow doing just that. rseq signal
delivery and return to usermode code could then ensure that high bits are
cleared by 32-bit tasks for all fields and thus provide a consistent
behavior for 32-bit tasks running on 32-bit and 64-bit kernels.

Thoughts ?

Thanks,

Mathieu

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