| Message ID | 20200226063551.65363-9-ruscur@russell.cc (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | set_memory() routines and STRICT_MODULE_RWX | expand |
On 26/2/20 5:35 pm, Russell Currey wrote: > There are a couple of reasons that the set_memory() functions are > problematic when STRICT_KERNEL_RWX isn't enabled: > > - The linear mapping is a different size and apply_to_page_range() > may modify a giant section, breaking everything > - patch_instruction() doesn't know to work around a page being marked > RO, and will subsequently crash > > The latter can be replicated by building a kernel with the set_memory() > patches but with STRICT_KERNEL_RWX off and running ftracetest. > > Reported-by: Jordan Niethe <jniethe5@gmail.com> > Signed-off-by: Russell Currey <ruscur@russell.cc> Can we squash this in earlier in the series for the sake of bisectability?
Le 26/02/2020 à 07:35, Russell Currey a écrit : > There are a couple of reasons that the set_memory() functions are > problematic when STRICT_KERNEL_RWX isn't enabled: > > - The linear mapping is a different size and apply_to_page_range() > may modify a giant section, breaking everything > - patch_instruction() doesn't know to work around a page being marked > RO, and will subsequently crash > > The latter can be replicated by building a kernel with the set_memory() > patches but with STRICT_KERNEL_RWX off and running ftracetest. I agree with Andrew, those changes should go into patch 1. > > Reported-by: Jordan Niethe <jniethe5@gmail.com> > Signed-off-by: Russell Currey <ruscur@russell.cc> > --- > v5: Apply to both set_memory_attr() and change_memory_attr() See my comments of v4, additional comments below > v4: New > > arch/powerpc/mm/pageattr.c | 22 ++++++++++++++++------ > 1 file changed, 16 insertions(+), 6 deletions(-) > > diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c > index ee6b5e3b7604..49b8e2e0581d 100644 > --- a/arch/powerpc/mm/pageattr.c > +++ b/arch/powerpc/mm/pageattr.c > @@ -64,13 +64,18 @@ static int change_page_attr(pte_t *ptep, unsigned long addr, void *data) > > int change_memory_attr(unsigned long addr, int numpages, long action) > { > - unsigned long start = ALIGN_DOWN(addr, PAGE_SIZE); > - unsigned long sz = numpages * PAGE_SIZE; > + unsigned long start, size; > + > + if (!IS_ENABLED(CONFIG_STRICT_KERNEL_RWX)) > + return 0; > > if (!numpages) > return 0; > > - return apply_to_page_range(&init_mm, start, sz, change_page_attr, (void *)action); > + start = ALIGN_DOWN(addr, PAGE_SIZE); > + size = numpages * PAGE_SIZE; > + > + return apply_to_page_range(&init_mm, start, size, change_page_attr, (void *)action); You don't need to move start and sz initialisation, neither you need to change the name of sz to size. If you want to rename sz to size, do it in the initial patch, but take care of the length of the lines. IIRC I used a short name to have the line fit on a single line with no more than 90 chars. Christophe > } > > /* > @@ -96,12 +101,17 @@ static int set_page_attr(pte_t *ptep, unsigned long addr, void *data) > > int set_memory_attr(unsigned long addr, int numpages, pgprot_t prot) > { > - unsigned long start = ALIGN_DOWN(addr, PAGE_SIZE); > - unsigned long sz = numpages * PAGE_SIZE; > + unsigned long start, size; > + > + if (!IS_ENABLED(CONFIG_STRICT_KERNEL_RWX)) > + return 0; > > if (!numpages) > return 0; > > - return apply_to_page_range(&init_mm, start, sz, set_page_attr, > + start = ALIGN_DOWN(addr, PAGE_SIZE); > + size = numpages * PAGE_SIZE; > + > + return apply_to_page_range(&init_mm, start, size, set_page_attr, > (void *)pgprot_val(prot)); > } >
diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c index ee6b5e3b7604..49b8e2e0581d 100644 --- a/arch/powerpc/mm/pageattr.c +++ b/arch/powerpc/mm/pageattr.c @@ -64,13 +64,18 @@ static int change_page_attr(pte_t *ptep, unsigned long addr, void *data) int change_memory_attr(unsigned long addr, int numpages, long action) { - unsigned long start = ALIGN_DOWN(addr, PAGE_SIZE); - unsigned long sz = numpages * PAGE_SIZE; + unsigned long start, size; + + if (!IS_ENABLED(CONFIG_STRICT_KERNEL_RWX)) + return 0; if (!numpages) return 0; - return apply_to_page_range(&init_mm, start, sz, change_page_attr, (void *)action); + start = ALIGN_DOWN(addr, PAGE_SIZE); + size = numpages * PAGE_SIZE; + + return apply_to_page_range(&init_mm, start, size, change_page_attr, (void *)action); } /* @@ -96,12 +101,17 @@ static int set_page_attr(pte_t *ptep, unsigned long addr, void *data) int set_memory_attr(unsigned long addr, int numpages, pgprot_t prot) { - unsigned long start = ALIGN_DOWN(addr, PAGE_SIZE); - unsigned long sz = numpages * PAGE_SIZE; + unsigned long start, size; + + if (!IS_ENABLED(CONFIG_STRICT_KERNEL_RWX)) + return 0; if (!numpages) return 0; - return apply_to_page_range(&init_mm, start, sz, set_page_attr, + start = ALIGN_DOWN(addr, PAGE_SIZE); + size = numpages * PAGE_SIZE; + + return apply_to_page_range(&init_mm, start, size, set_page_attr, (void *)pgprot_val(prot)); }
There are a couple of reasons that the set_memory() functions are problematic when STRICT_KERNEL_RWX isn't enabled: - The linear mapping is a different size and apply_to_page_range() may modify a giant section, breaking everything - patch_instruction() doesn't know to work around a page being marked RO, and will subsequently crash The latter can be replicated by building a kernel with the set_memory() patches but with STRICT_KERNEL_RWX off and running ftracetest. Reported-by: Jordan Niethe <jniethe5@gmail.com> Signed-off-by: Russell Currey <ruscur@russell.cc> --- v5: Apply to both set_memory_attr() and change_memory_attr() v4: New arch/powerpc/mm/pageattr.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-)