Messages in this thread | | | From | Andy Lutomirski <> | Date | Thu, 15 Feb 2018 20:02:47 +0000 | Subject | Re: [PATCH RFC v2 4/6] x86: Disable PTI on compatibility mode |
| |
On Thu, Feb 15, 2018 at 4:36 PM, Nadav Amit <namit@vmware.com> wrote: > Based on the understanding that there should be no way for userspace to > address the kernel-space from compatibility mode, disable it while > running in compatibility mode as long as the 64-bit code segment of the > user is not used. > > Reenabling PTI is performed by restoring NX-bits to the userspace > mappings, flushing the TLBs, and notifying all the CPUs that use the > affected mm to disable PTI. Each core responds by removing the present > bit for the 64-bit code-segment, and marking that PTI is disabled on > that core. >
I dislike this patch because it's conflating two things. The patch claims to merely disable PTI for compat tasks, whatever those are. But it's also introducing a much stronger concept of what a compat task is. The kernel currently mostly doesn't care whether a task is "compat" or not, and I think that most remaining code paths that do care are buggy and should be removed.
I think the right way to approach this is to add a new arch_prctl() that changes allowable bitness, like this:
arch_prctl(ARCH_SET_ALLOWED_GDT_CS, X86_ALLOW_CS32 | X86_ALLOW_CS64);
this would set the current task to work the normal way, where 32-bit and 64-bit CS are available. You could set just X86_ALLOW_CS32 to deny 64-bit mode and just X86_ALLOW_CS64 to deny 32-bit mode. This would make nice attack surface reduction tools for the more paranoid sandbox users to use. Doing arch_prctl(ARCH_SET_ALLOWED_GDT_CS, 0) would return -EINVAL.
A separate patch would turn PTI off if you set X86_ALLOW_CS32.
This has the downside that old code doesn't get the benefit without some code change, but that's not the end of the world.
> +static void pti_cpu_update_func(void *info) > +{ > + struct mm_struct *mm = (struct mm_struct *)info; > + > + if (mm != this_cpu_read(cpu_tlbstate.loaded_mm)) > + return; > + > + /* > + * Keep CS64 and CPU settings in sync despite potential concurrent > + * updates. > + */ > + set_cpu_pti_disable(READ_ONCE(mm->context.pti_disable)); > +}
I don't like this at all. IMO a sane implementation should never change PTI status on a remote CPU. Just track it per task.
> +void __pti_reenable(void) > +{ > + struct mm_struct *mm = current->mm; > + int cpu; > + > + if (!mm_pti_disable(mm)) > + return; > + > + /* > + * Prevent spurious page-fault storm while we set the NX-bit and have > + * yet not updated the per-CPU pti_disable flag. > + */ > + down_write(&mm->mmap_sem); > + > + if (!mm_pti_disable(mm)) > + goto out; > + > + /* > + * First, mark the PTI is enabled. Although we do anything yet, we are > + * safe as long as we do not reenable CS64. Since we did not update the > + * page tables yet, this may lead to spurious page-faults, but we need > + * the pti_disable in mm to be set for __pti_set_user_pgd() to do the > + * right thing. Holding mmap_sem would ensure matter we hold the > + * mmap_sem to prevent them from swamping the system. > + */ > + mm->context.pti_disable = PTI_DISABLE_OFF; > + > + /* Second, restore the NX bits. */ > + pti_update_user_pgds(mm, true);
You're holding mmap_sem, but there are code paths that touch page tables that don't hold mmap_sem, such as the stack extension code.
> + > +bool pti_handle_segment_not_present(long error_code) > +{ > + if (!static_cpu_has(X86_FEATURE_PTI)) > + return false; > + > + if ((unsigned short)error_code != GDT_ENTRY_DEFAULT_USER_CS << 3) > + return false; > + > + pti_reenable(); > + return true; > +}
Please don't. You're trying to emulate the old behavior here, but you're emulating it wrong. In particular, you won't trap on LAR.
| |