Messages in this thread | | | Subject | Re: [patch V3 03/11] x86/mm/cpa: Rework static_protections() | From | Dave Hansen <> | Date | Fri, 21 Sep 2018 09:33:08 -0700 |
| |
On 09/17/2018 07:29 AM, Thomas Gleixner wrote: > +/* > + * The kernel text needs to be executable for obvious reasons. This does > + * not cover __inittext since that is gone after boot. On 64bit we do not > + * enforce !NX on the low mapping > + */ > +static pgprotval_t protect_kernel_text(unsigned long address) > +{ > + if (within(address, (unsigned long)_text, (unsigned long)_etext)) > + return _PAGE_NX; > + return 0; > +}
Minor nit: I was scratching my head about how why this works. It _reads_ like we are using _PAGE_NX to protect kernel text which doesn't make any sense of course.
Could we make a connection between the protection and _PAGE_NX in the comments:
Protect kernel text against by forbidding _PAGE_NX. This protects only the high kernel mapping (_text -> _etext) out of which we actually execute. Do not protect the low mapping.
This does not cover __inittext since that is gone after boot.
The static_protections() code looks fine because it's totally obvious that it is dealing with "forbidden" bits, btw:
> +static inline pgprot_t static_protections(pgprot_t prot, unsigned long address, > + unsigned long pfn) > +{ > + pgprotval_t forbidden; > + > + /* Operate on the virtual address */ > + forbidden = protect_kernel_text(address); > + forbidden |= protect_kernel_text_ro(address); > + > + /* Check the PFN directly */ > + forbidden |= protect_pci_bios(pfn); > + forbidden |= protect_rodata(pfn); > > - return prot; > + return __pgprot(pgprot_val(prot) & ~forbidden); > }
This is more of a, "if you happen to respin these" comment, though, so:
Reviewed-by: Dave Hansen <dave.hansen@intel.com>
| |