On Wed, Nov 22, 2017 at 8:07 PM, Andy Lutomirski <luto@xxxxxxxxxx> wrote:
> On Wed, Nov 22, 2017 at 4:34 PM, Dave Hansen
> <dave.hansen@xxxxxxxxxxxxxxx> wrote:
>>
>> These actions when dealing with a user address *and* the
>> PGD has _PAGE_USER set. That way, in-kernel users of low addresses
>> typically used by userspace are not accidentally poisoned.
>
> This seems sane.
>
>> +/*
>> + * Take a PGD location (pgdp) and a pgd value that needs
>> + * to be set there. Populates the shadow and returns
>> + * the resulting PGD that must be set in the kernel copy
>> + * of the page tables.
>> + */
>> +static inline pgd_t kaiser_set_shadow_pgd(pgd_t *pgdp, pgd_t pgd)
>> +{
>> +#ifdef CONFIG_KAISER
>> + if (pgd_userspace_access(pgd)) {
>> + if (pgdp_maps_userspace(pgdp)) {
>> + /*
>> + * The user/shadow page tables get the full
>> + * PGD, accessible from userspace:
>> + */
>> + kernel_to_shadow_pgdp(pgdp)->pgd = pgd.pgd;
>> + /*
>> + * For the copy of the pgd that the kernel
>> + * uses, make it unusable to userspace. This
>> + * ensures if we get out to userspace with the
>> + * wrong CR3 value, userspace will crash
>> + * instead of running.
>> + */
>> + pgd.pgd |= _PAGE_NX;
>> + }
>> + } else if (pgd_userspace_access(*pgdp)) {
>> + /*
>> + * We are clearing a _PAGE_USER PGD for which we
>> + * presumably populated the shadow. We must now
>> + * clear the shadow PGD entry.
>> + */
>> + if (pgdp_maps_userspace(pgdp)) {
>> + kernel_to_shadow_pgdp(pgdp)->pgd = pgd.pgd;
>> + } else {
>> + /*
>> + * Attempted to clear a _PAGE_USER PGD which
>> + * is in the kernel porttion of the address
>> + * space. PGDs are pre-populated and we
>> + * never clear them.
>> + */
>> + WARN_ON_ONCE(1);
>> + }
>> + } else {
>> + /*
>> + * _PAGE_USER was not set in either the PGD being set
>> + * or cleared. All kernel PGDs should be
>> + * pre-populated so this should never happen after
>> + * boot.
>> + */
>> + }
>> +#endif
>> + /* return the copy of the PGD we want the kernel to use: */
>> + return pgd;
>> +}
>> +
>
> The more I read this code, the more I dislike "shadow". Shadow
> pagetables mean something specific in the virtualization world and,
> more importantly, the word "shadow" fails to convey *which* table it
> is. Unless I'm extra confused, mm->pgd points to the kernelmode
> tables. So can we replace the word "shadow" with "usermode"? That
> will also make the entry stuff way clearer. (Or I have it backwards,
> in which case "kernelmode" would be the right choice.) And rename the
> argument.
>
> That confusion aside, I'm trying to wrap my head around this. I think
> the description above makes sense, but I'm struggling to grok the code
> and how it matches the description. May I suggest an alternative
> implementation? (Apologies for epic whitespace damage.)
>
> /*
> * Install an entry into the usermode pgd. pgdp points to the kernelmode
> * entry whose usermode counterpart we're supposed to set. pgd is the
> * desired entry. Returns pgd, possibly modified if the actual entry installed
> * into the kernelmode needs different mode bits.
> */
> static inline pgd_t kaiser_set_usermode_pgd(pgd_t *pgdp, pgd_t pgd) {
> VM_BUG_ON(pgdp points to a usermode table);
>
> if (pgdp_maps_userspace(pgdp)) {
> /* Install the pgd as requested into the usermode tables. */
> kernelmode_to_usermode_pgdp(pgdp)->pgd = pgd.pgd;
>
> if (pgd_val(pgd) & _PAGE_USER) {
> /*
> * This is a normal user pgd -- the kernelmode mapping should have NX
> * set to prevent erroneous usermode execution with the kernel tables.
> */
> return __pgd(pgd_val(pgd) | _PAGE_NX;
> } else {
> /* This is a weird mapping, e.g. EFI. Map it straight through. */
> return pgd;
> }
> } else {
> /*
> * We can get here due to vmalloc, a vmalloc fault, memory
> hot-add, or initial setup
> * of kernelmode page tables. Regardless of which particular code
> path we're in,
> * these mappings should not be automatically propagated to the
> usermode tables.
> */
> return pgd;
> }
> }
>
> As a side benefit, this shouldn't have magical interactions with the
> vsyscall page any more.
>
> Are there cases that this would get wrong?
>
Quick ping: did this get lost?
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
