On 28/07/2026 18:46, Petr Pavlu wrote:
> 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: [email protected]
>> Signed-off-by: Jiacheng Yu <[email protected]>
>
> 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.
Thanks. That is the intent of this patch.
>
>> ---
>> 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?
Functionally, plain strcpy() is fine here. The preceding
strnlen(val, maxlen + 1) either finds the NUL byte within the limit or
rejects the string, and the new allocation is exactly len + 1 bytes.
However, when this patch rewrites the line to use tmp,
checkpatch --strict reports the following warning:
WARNING: Prefer strscpy over strcpy
Since the line is being rewritten anyway, I changed strcpy() to
strscpy() to follow that preference:
https://github.com/KSPP/linux/issues/88
>
>> } 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].
I agree. Writable charp parameters are expected to be protected by
kernel_param_lock(), and this patch does not change that locking model.
>
>>
>> return 0;
>> }
>
> [1]
> https://lore.kernel.org/linux-modules/[email protected]/
> [2]
> https://github.com/torvalds/linux/blob/v7.2-rc5/include/linux/moduleparam.h#L124
>
