[PATCH] efi: pstore: Fix pstore_disable parameter setting (if built as module)

[PATCH] efi: pstore: Fix pstore_disable parameter setting (if built as module)

[Guilherme G. Piccoli]

Commit a28655c330ab ("efi: pstore: Allow dynamic initialization based on module parameter")
introduced the functionality of switching the pstore_disable parameter,
effectively allowing efi-pstore to be used even if builtin and having
pstore_disabled=Y (a lot of distros do that at Kconfig level).

Well, it also introduced a bug if efi-pstore is built as module and loaded
with pstore_disable=N parameter. On load time, load_module() -> parse_args()
calls our mod_param callback efi_pstore_disable_set(), which initializes
efi_pstore, registering it as a pstore backend. Happens that, slightly
later in the module loading process, do_init_module() calls our module
init function, which currently ... retries registering us as a pstore
backend. Since this was done successfully before, in the module parameter
parsing, pstore_register() refuses to re-register us and efi-pstore ends-up
clearing some structures, like freeing its data buffer allocated memory
and setting its size to 0.

Net effect of that: efi-pstore ends-up in a weird state, partially
initialized but with cleared structures. More than that, if we try to
unload it, it won't call pstore_unregister() but will get the module
unloaded, hence pstore writes on oops or with other frontends will
trigger NULL pointer dereferences.

Fix it by guarding efivars_pstore_init() against re-running if the
driver was previously initialized - this is kind of an analog to
what is already done on efivars_pstore_exit() and fixes the issue.

Fixes: a28655c330ab ("efi: pstore: Allow dynamic initialization based on module parameter")
Cc: stable@vger.kernel.org
Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
---


Hi folks, apologies for introducing this bug =|
Cheers,


Guilherme


 drivers/firmware/efi/efi-pstore.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
index a5db3534f0a6..2e125db7d43b 100644
--- a/drivers/firmware/efi/efi-pstore.c
+++ b/drivers/firmware/efi/efi-pstore.c
@@ -263,6 +263,9 @@ static int efivars_pstore_init(void)
 	if (pstore_disable)
 		return 0;
 
+	if (efi_pstore_info.bufsize)
+		return 0;
+
 	/*
 	 * Notice that 1024 is the minimum here to prevent issues with
 	 * decompression algorithms that were spotted during tests;
-- 
2.50.1

Re: [PATCH] efi: pstore: Fix pstore_disable parameter setting (if built as module)

[Ard Biesheuvel]

Hello Guilherme,

On Mon, 7 Sep 2026, at 01:55, Guilherme G. Piccoli wrote:
> Commit a28655c330ab ("efi: pstore: Allow dynamic initialization based 
> on module parameter")
> introduced the functionality of switching the pstore_disable parameter,
> effectively allowing efi-pstore to be used even if builtin and having
> pstore_disabled=Y (a lot of distros do that at Kconfig level).
>
> Well, it also introduced a bug if efi-pstore is built as module and loaded
> with pstore_disable=N parameter. On load time, load_module() -> parse_args()
> calls our mod_param callback efi_pstore_disable_set(), which initializes
> efi_pstore, registering it as a pstore backend. Happens that, slightly
> later in the module loading process, do_init_module() calls our module
> init function, which currently ... retries registering us as a pstore
> backend. Since this was done successfully before, in the module parameter
> parsing, pstore_register() refuses to re-register us and efi-pstore ends-up
> clearing some structures, like freeing its data buffer allocated memory
> and setting its size to 0.
>
> Net effect of that: efi-pstore ends-up in a weird state, partially
> initialized but with cleared structures. More than that, if we try to
> unload it, it won't call pstore_unregister() but will get the module
> unloaded, hence pstore writes on oops or with other frontends will
> trigger NULL pointer dereferences.
>
> Fix it by guarding efivars_pstore_init() against re-running if the
> driver was previously initialized - this is kind of an analog to
> what is already done on efivars_pstore_exit() and fixes the issue.
>
> Fixes: a28655c330ab ("efi: pstore: Allow dynamic initialization based 
> on module parameter")
> Cc: stable@vger.kernel.org
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
> ---
>
>
> Hi folks, apologies for introducing this bug =|

No worries :-)

>
>  drivers/firmware/efi/efi-pstore.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/firmware/efi/efi-pstore.c 
> b/drivers/firmware/efi/efi-pstore.c
> index a5db3534f0a6..2e125db7d43b 100644
> --- a/drivers/firmware/efi/efi-pstore.c
> +++ b/drivers/firmware/efi/efi-pstore.c
> @@ -263,6 +263,9 @@ static int efivars_pstore_init(void)
>  	if (pstore_disable)
>  		return 0;
> 
> +	if (efi_pstore_info.bufsize)
> +		return 0;
> +

Can we please address the root cause rather than paper over it here?

What is the point of having a) a pstore_disable= param on a module
(which can be blacklisted or simply not loaded) and b) having it
runtime writable so pstore-efi can be toggled on/off at will?

Also, the practice of controlling the functionality of a builtin
diagnostic feature by poking something under /sys/module/foo/params
is in rather poor taste IMNSHO.




>  	/*
>  	 * Notice that 1024 is the minimum here to prevent issues with
>  	 * decompression algorithms that were spotted during tests;
> -- 
> 2.50.1

Re: [PATCH] efi: pstore: Fix pstore_disable parameter setting (if built as module)

[Guilherme G. Piccoli]

Hi Ard, thanks for your prompt response!


>> [...]
>>  	if (pstore_disable)
>>  		return 0;
>>
>> +	if (efi_pstore_info.bufsize)
>> +		return 0;
>> +
> 
> Can we please address the root cause rather than paper over it here?
> 
> What is the point of having a) a pstore_disable= param on a module
> (which can be blacklisted or simply not loaded) and b) having it
> runtime writable so pstore-efi can be toggled on/off at will?
> 

Well ... despite we agree on that, I think removing pstore_disable is
kinda breaking "the contract" we're following since long time ago.

The thing is: unlike ramoops, or even maybe pstore/blk and other
potential backends, efi_pstore is a bit of a different beast IIUC.

It doesn't require any config, by having it loaded, it'll just go ahead
and ...get enabled as pstore backend (unless pstore.backend sets otherwise).

And not only that: efi_pstore is **usually built-in** - I guess in all
distros I've checked and by default config, it's built-in.

So, probably to avoid having to use {initcall,module}_blacklist
(depending on the build type), I guess it was decided to have this
parameter. The commit [0] that added that, >13y ago, mentions that since
it was enabled by default and some EFI implementations had issues when
filling its memory with oops logs, the option was to add such
parameter+Kconfig to force disabling efi_pstore. Good decision or not,
we should now live with that IMO.


> Also, the practice of controlling the functionality of a builtin
> diagnostic feature by poking something under /sys/module/foo/params
> is in rather poor taste IMNSHO.
> 

Not only poor taste, I would say super counter-intuitive. But allowing
the sysfs playing was added by myself, to enable users to change it
without requiring a reboot, for a very cheap cost (well...not so cheap,
since I added a bug and here am I now, trying to fix it heh).

If we require a reboot, we are in the same situation of kdump -
crashkernel must be set on command-line and requires a reboot to
properly reserve the memory. There is no technical reason for that in
case of efi_pstore, at least, none I can see.


So, with all that said, what is the path you suggest we follow?

a) This simple fix (or something similar)?

b) Drop entirely pstore_disable, risking a lot of users to just face
breakages in their setups?

c) Other ideas?


Appreciate your/others feedback, thanks in advance!
Cheers,

Guilherme


[0] ec0971ba5372 ("efivars: Add module parameter to disable use as a
pstore backend")