----- On Jun 29, 2018, at 4:39 PM, Andy Lutomirski luto@xxxxxxxxxxxxxx wrote: > On Fri, Jun 29, 2018 at 12:48 PM, Mathieu Desnoyers > <mathieu.desnoyers@xxxxxxxxxxxx> 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. > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-api" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
