On Wed, Nov 22, 2017 at 8:07 PM, Andy Lutomirski <l...@kernel.org> wrote: > On Wed, Nov 22, 2017 at 4:34 PM, Dave Hansen > <dave.han...@linux.intel.com> 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?