Re: [PATCH v5 8/8] powerpc/mm: Disable set_memory() routines when strict RWX isn't enabled

[Christophe Leroy <christophe.leroy@c-s.fr>]

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));
>   }
>