lkml.org 
[lkml]   [2018]   [Jul]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [RFC PATCH for 4.18 1/2] rseq: validate rseq_cs fields are < TASK_SIZE
    On Mon, Jul 2, 2018 at 7:32 AM Mathieu Desnoyers
    <mathieu.desnoyers@efficios.com> wrote:
    >
    > ----- 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
    > > <mathieu.desnoyers@efficios.com> wrote:
    > >> 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.
    >

    TIF_NOTIFY_RESUME context in the exit slowpath is fine, too.

    > >
    > > 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 ?

    It's not about security. The idea is that instruction_pointer_set()
    should return some indication of whether it actually set the
    instruction pointer to the requested value. On x86, if you have
    !user_64bit_mode(regs) and you call instruction_pointer_set() to set
    ip to 0xbaadc0de12345678, then you end up with a state where we will
    probably execute user code at the address 0x12345678. Conversely, if
    you have user_64bit_mode(regs) == true and you set ip to
    0xbaadc0de12345678, then you will end up sending a signal to the task
    because 0xbaadc0de12345678 is not executable (and, in fact, is highly
    likely to be noncanonical).

    So I would argue that the semantics *should* be:

    /*
    * Attempts to modify @regs such that the next user instruction to be
    executed is
    * the instruction at @addr. instruction_pointer_set() may return
    false to indicate
    * that addr was invalid in the sense that the next user instruction executed
    * might be some other address instead. The most likely cause is that
    * regs refers to a 32-bit compat context, addr != (u32)addr, and the
    architecture
    * might silently truncate the address on the next return to user code.
    *
    * instruction_pointer_set() must only be called from a context in
    which the architecture
    * allows arbitrary modifications of @regs.
    *
    * Architecture implementations promise that calling
    instruction_pointer_set() will not
    * crash or otherwise corrupt the kernel when called from a valid
    context, regardless
    * of what value is passed in @addr.
    */
    bool instruction_pointer_set(struct pt_regs *regs, unsigned long addr);

    >
    > >
    > > (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.

    Just use the 64-bit values, I think. I see no point in killing the task.


    >
    > 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.

    Agreed.

    >
    > 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.

    I would be okay with that, too, but I think it would have to be
    user_64bit_mode(task, regs), since
    sane architectures would have the task bitness somewhere other than in
    regs. x86 is IMO rather
    weird in this regard. When I added user_64bit_mode(), I didn't
    envision its use outside x86 arch code.

    > AFAIU this could be achieved by re-introducing is_compat_task() on x86 as:
    >
    > #ifdef CONFIG_COMPAT
    > static bool is_compat_task(void)
    > {
    > return user_64bit_mode(current_pt_regs()));
    > }
    > #else
    > static bool is_compat_task(void) { return false; };
    > #endif
    >
    > Or am I missing something ?

    is_compat_task() historically literally meant "am I in a compat system
    call". It never worked consistently on x86 outside of syscall
    context. While I do have fundamental objections to having a generic
    concept of "is this a compat task?" on Linux, that's not why I removed
    is_compat_task(). I removed it because it didn't do what the name
    suggested.

    Unfortunately, while it's gone from generic code, it's still there on
    non-x86 arches, and it probably still has inconsistent semantics. So
    I don't want to re-add it.

    But I think that the limited solution of changing
    instruction_pointer_set() really is a sufficient
    architecture-dependent change to fully solve your problem.

    \
     
     \ /
      Last update: 2018-07-02 19:13    [W:5.155 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site