Re: [PATCH] params: fix charp corruption on allocation failure

[Petr Pavlu <petr.pavlu@suse.com>]

On 7/28/26 10:55 AM, Jiacheng Yu wrote:
> param_set_charp() stores charp parameters in allocated memory after slab is
> available, and releases the previous value when the parameter is updated.
> 
> The previous value is released before the replacement allocation succeeds.
> If kmalloc_parameter() fails, the setter returns -ENOMEM with the parameter
> left as NULL.
> 
> Failing zswap's compressor update before zswap is initialized can later
> trigger:
> 
>   BUG: kernel NULL pointer dereference, address: 0000000000000000
>   RIP: 0010:strcmp+0x10/0x30
>   Call Trace:
>     zswap_setup+0x3b1/0x490
>     zswap_enabled_param_set+0x5b/0xa0
>     param_attr_store+0x93/0xe0
>     module_attr_store+0x1c/0x30
>     kernfs_fop_write_iter+0x116/0x1f0
> 
> Allocate and copy the replacement first, then replace the parameter value
> only after allocation succeeds.
> 
> Fixes: e180a6b7759a ("param: fix charp parameters set via sysfs")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jiacheng Yu <yujiacheng3@huawei.com>

It makes sense to me for param_set_charp() to have commit-or-rollback
semantics. The set callbacks of other standard parameters behave this
way, with the exception of array parameters.

> ---
>  kernel/params.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/params.c b/kernel/params.c
> index a668863a4bb6..e4f2b71dde1e 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -261,6 +261,7 @@ EXPORT_SYMBOL_GPL(param_set_uint_minmax);
>  
>  int param_set_charp(const char *val, const struct kernel_param *kp)
>  {
> +	char *tmp;
>  	size_t len, maxlen = 1024;
>  
>  	len = strnlen(val, maxlen + 1);
> @@ -269,19 +270,20 @@ int param_set_charp(const char *val, const struct kernel_param *kp)
>  		return -ENOSPC;
>  	}
>  
> -	maybe_kfree_parameter(*(char **)kp->arg);
> -
>  	/*
>  	 * This is a hack. We can't kmalloc() in early boot, and we
>  	 * don't need to; this mangled commandline is preserved.
>  	 */
>  	if (slab_is_available()) {
> -		*(char **)kp->arg = kmalloc_parameter(len + 1);
> -		if (!*(char **)kp->arg)
> +		tmp = kmalloc_parameter(len + 1);
> +		if (!tmp)
>  			return -ENOMEM;
> -		strcpy(*(char **)kp->arg, val);
> +		strscpy(tmp, val, len + 1);

What's wrong with the plain strcpy() here?

>  	} else
> -		*(const char **)kp->arg = val;
> +		tmp = (char *)val;
> +
> +	maybe_kfree_parameter(*(char **)kp->arg);
> +	*(char **)kp->arg = tmp;

Sashiko reports [1] that there is a pre-existing use-after-free window
between freeing the old parameter and updating kp->arg. However, this
issue doesn't appear to be valid because any concurrent access to
a writable charp parameter should be protected by kernel_param_lock().
This is documented include/linux/moduleparam.h [2].

>  
>  	return 0;
>  }

[1] https://lore.kernel.org/linux-modules/20260728075803.AA13C1F000E9@smtp.kernel.org/
[2] https://github.com/torvalds/linux/blob/v7.2-rc5/include/linux/moduleparam.h#L124

-- 
Thanks,
Petr