linux-kernel - Re: [PATCH 2/2] powerpc: Don't ignore errors from set_memory_{n}p() in __kernel_map

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 21 Feb 2024 23:09:43 +1100
From: Michael Ellerman <mpe@...erman.id.au>
To: Christophe Leroy <christophe.leroy@...roup.eu>, Nicholas Piggin
 <npiggin@...il.com>
Cc: Christophe Leroy <christophe.leroy@...roup.eu>,
 linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH 2/2] powerpc: Don't ignore errors from set_memory_{n}p()
 in __kernel_map_pages()

Christophe Leroy <christophe.leroy@...roup.eu> writes:
> set_memory_p() and set_memory_np() can fail.
>
> As mentioned in linux/mm.h:
>
> /*
>  * To support DEBUG_PAGEALLOC architecture must ensure that
>  * __kernel_map_pages() never fails
>  */
>
> So panic in case set_memory_p() or set_memory_np() fail
> in __kernel_map_pages().
>
> Link: https://github.com/KSPP/linux/issues/7
> Signed-off-by: Christophe Leroy <christophe.leroy@...roup.eu>
> ---
>  arch/powerpc/include/asm/book3s/64/hash.h |  2 +-
>  arch/powerpc/mm/book3s64/hash_utils.c     |  3 ++-
>  arch/powerpc/mm/pageattr.c                | 10 +++++++---
>  3 files changed, 10 insertions(+), 5 deletions(-)
>
..
> diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c
> index 16b8d20d6ca8..62b678585878 100644
> --- a/arch/powerpc/mm/pageattr.c
> +++ b/arch/powerpc/mm/pageattr.c
> @@ -106,17 +106,21 @@ int change_memory_attr(unsigned long addr, int numpages, long action)
>  #ifdef CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC
>  void __kernel_map_pages(struct page *page, int numpages, int enable)
>  {
> +	int err;
>  	unsigned long addr = (unsigned long)page_address(page);
>  
>  	if (PageHighMem(page))
>  		return;
>  
>  	if (IS_ENABLED(CONFIG_PPC_BOOK3S_64) && !radix_enabled())
> -		hash__kernel_map_pages(page, numpages, enable);
> +		err = hash__kernel_map_pages(page, numpages, enable);
>  	else if (enable)
> -		set_memory_p(addr, numpages);
> +		err = set_memory_p(addr, numpages);
>  	else
> -		set_memory_np(addr, numpages);
> +		err = set_memory_np(addr, numpages);
> +
> +	if (err)
> +		panic("%s: set_memory_%sp() failed\n", enable ? "" : "n");

This doesn't compile, it's missing __func__ I guess.

Seems like we could keep it simpler though, it should hopefully never
happen anyway, eg:

  panic("%s: changing memory protections failed\n", __func__);


cheers