[PATCH] params: fix charp corruption on allocation failure

[PATCH] params: fix charp corruption on allocation failure

[Jiacheng Yu]

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>
---
 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);
 	} else
-		*(const char **)kp->arg = val;
+		tmp = (char *)val;
+
+	maybe_kfree_parameter(*(char **)kp->arg);
+	*(char **)kp->arg = tmp;
 
 	return 0;
 }
-- 
2.34.1

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

[Petr Pavlu]

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

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

[Jiacheng Yu]

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: 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.

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/20260728075803.AA13C1F000E9@smtp.kernel.org/
> [2] https://github.com/torvalds/linux/blob/v7.2-rc5/include/linux/moduleparam.h#L124
> 

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

[Petr Pavlu]

On 7/28/26 2:22 PM, Jiacheng Yu wrote:
> On 28/07/2026 18:46, Petr Pavlu wrote:
>> On 7/28/26 10:55 AM, Jiacheng Yu wrote:
>>> 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

I don't see a benefit to using strscpy() here. The length of the string
is already known and the tmp buffer is correctly sized, so using
`memcpy(tmp, val, len + 1)` seems most appropriate to me.

-- 
Thanks,
Petr

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

[Jiacheng Yu]

On 28/07/2026 20:57, Petr Pavlu wrote:
> On 7/28/26 2:22 PM, Jiacheng Yu wrote:
>> On 28/07/2026 18:46, Petr Pavlu wrote:
>>> On 7/28/26 10:55 AM, Jiacheng Yu wrote:
>>>> 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
> 
> I don't see a benefit to using strscpy() here. The length of the string
> is already known and the tmp buffer is correctly sized, so using
> `memcpy(tmp, val, len + 1)` seems most appropriate to me.

Agreed, memcpy() is more appropriate here since the length is already
known. Changed in v2, thanks for the review.

> 

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

[Jiacheng Yu]

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>
---

v1 -> v2:
- Use memcpy() instead of strscpy() as the length is already known.

 kernel/params.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/kernel/params.c b/kernel/params.c
index a668863a4bb6..939ef84a46ff 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);
+		memcpy(tmp, val, len + 1);
 	} else
-		*(const char **)kp->arg = val;
+		tmp = (char *)val;
+
+	maybe_kfree_parameter(*(char **)kp->arg);
+	*(char **)kp->arg = tmp;
 
 	return 0;
 }
-- 
2.34.1

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

[Petr Pavlu]

On 7/29/26 2:32 PM, 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>

Looks ok to me.

Reviewed-by: Petr Pavlu <petr.pavlu@suse.com>

I'll give others more time to comment. If there are no further concerns,
I plan to take this on modules-fixes.

-- 
Thanks,
Petr

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

[Petr Pavlu]

On 7/31/26 4:21 PM, Petr Pavlu wrote:
> On 7/29/26 2:32 PM, 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>
> 
> Looks ok to me.
> 
> Reviewed-by: Petr Pavlu <petr.pavlu@suse.com>
> 
> I'll give others more time to comment. If there are no further concerns,
> I plan to take this on modules-fixes.

I've queued the fix on modules-next for v7.3-rc1.

-- 
Thanks,
Petr