Re: [PATCH 1/3] init: Declare rodata_enabled and mark_rodata_ro() at all time

[Christophe Leroy <christophe.leroy@xxxxxxxxxx>]

Le 22/12/2023 à 06:35, Kees Cook a écrit :
> [Vous ne recevez pas souvent de courriers de kees@xxxxxxxxxx. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
> 
> On December 21, 2023 4:16:56 AM PST, Michael Ellerman <mpe@xxxxxxxxxxxxxx> wrote:
>> Cc +Kees
>>
>> Christophe Leroy <christophe.leroy@xxxxxxxxxx> writes:
>>> Declaring rodata_enabled and mark_rodata_ro() at all time
>>> helps removing related #ifdefery in C files.
>>>
>>> Signed-off-by: Christophe Leroy <christophe.leroy@xxxxxxxxxx>
>>> ---
>>>   include/linux/init.h |  4 ----
>>>   init/main.c          | 21 +++++++--------------
>>>   2 files changed, 7 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/include/linux/init.h b/include/linux/init.h
>>> index 01b52c9c7526..d2b47be38a07 100644
>>> --- a/include/linux/init.h
>>> +++ b/include/linux/init.h
>>> @@ -168,12 +168,8 @@ extern initcall_entry_t __initcall_end[];
>>>
>>>   extern struct file_system_type rootfs_fs_type;
>>>
>>> -#if defined(CONFIG_STRICT_KERNEL_RWX) || defined(CONFIG_STRICT_MODULE_RWX)
>>>   extern bool rodata_enabled;
>>> -#endif
>>> -#ifdef CONFIG_STRICT_KERNEL_RWX
>>>   void mark_rodata_ro(void);
>>> -#endif
>>>
>>>   extern void (*late_time_init)(void);
>>>
>>> diff --git a/init/main.c b/init/main.c
>>> index e24b0780fdff..807df08c501f 100644
>>> --- a/init/main.c
>>> +++ b/init/main.c
>>> @@ -1396,10 +1396,9 @@ static int __init set_debug_rodata(char *str)
>>>   early_param("rodata", set_debug_rodata);
>>>   #endif
>>>
>>> -#ifdef CONFIG_STRICT_KERNEL_RWX
>>>   static void mark_readonly(void)
>>>   {
>>> -    if (rodata_enabled) {
>>> +    if (IS_ENABLED(CONFIG_STRICT_KERNEL_RWX) && rodata_enabled) {
> 
> I think this will break without rodata_enabled actual existing on other architectures. (Only declaration was made visible, not the definition, which is above here and still behind ifdefs?)

The compiler constant-folds IS_ENABLED(CONFIG_STRICT_KERNEL_RWX).
When it is false, the second part is dropped.

Exemple:

bool test(void)
{
	if (IS_ENABLED(CONFIG_STRICT_KERNEL_RWX) && rodata_enabled)
		return true;
	else
		return false;
}

With CONFIG_STRICT_KERNEL_RWX set, it directly returns the content of 
rodata_enabled:

00000160 <test>:
  160:	3d 20 00 00 	lis     r9,0
			162: R_PPC_ADDR16_HA	rodata_enabled
  164:	88 69 00 00 	lbz     r3,0(r9)
			166: R_PPC_ADDR16_LO	rodata_enabled
  168:	4e 80 00 20 	blr

With CONFIG_STRICT_KERNEL_RWX unset, it returns 0 and doesn't reference 
rodata_enabled at all:

000000bc <test>:
   bc:	38 60 00 00 	li      r3,0
   c0:	4e 80 00 20 	blr

Many places in the kernel use this approach to minimise amount of #ifdefs.

Christophe