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]

Message-ID: <eefeb056-bc71-46ad-90b3-b07c392f5903@csgroup.eu>
Date: Wed, 21 Feb 2024 12:55:04 +0000
From: Christophe Leroy <christophe.leroy@...roup.eu>
To: Michael Ellerman <mpe@...erman.id.au>, Nicholas Piggin <npiggin@...il.com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linuxppc-dev@...ts.ozlabs.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()



Le 21/02/2024 à 13:09, Michael Ellerman a écrit :
> 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.

Damn, I was too quick when I took into account checkpatch's feedback, 
sorry for that.

> 
> Seems like we could keep it simpler though, it should hopefully never
> happen anyway, eg:
> 
>    panic("%s: changing memory protections failed\n", __func__);

Sure, let's do that. Do you want a v2 or you do the change directly ?

Thanks
Christophe