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

On 29/07/2026 19:38, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Use-After-Free race condition due to dangling global pointer during module parameter update.
> --
> 
> --- Patch [1]: [PATCH v2] params: fix charp corruption on allocation failure ---
> commit c59e6d7e9f9563e1394c57f9eedf07a9bb5f212b
> Author: Jiacheng Yu <[email protected]>
> 
> params: fix charp corruption on allocation failure
> 
> This patch fixes a corruption issue when param_set_charp() fails to allocate
> memory for the replacement string. It allocates and copies the replacement
> first, replacing the parameter value only after allocation succeeds.
> 
>> diff --git a/kernel/params.c b/kernel/params.c
>> index d3a4ecab4f278..8b25133fed242 100644
>> --- a/kernel/params.c
>> +++ b/kernel/params.c
> [ ... ]
>> @@ -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;
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does this sequence leave a
> deterministic use-after-free window for concurrent readers?
> 
> Because maybe_kfree_parameter() frees the old string while the global
> parameter variable still points to it, any concurrent lockless reader of
> the parameter might dereference a freed pointer before the reassignment
> on the next line.

As Petr pointed out, concurrent access to a writable charp parameter
is protected by kernel_param_lock(), as documented in
include/linux/moduleparam.h.No reorder is needed here.

> 
> Could this be safely reordered to update the pointer before freeing the old
> memory? For example:
> 
> 	char *old = *(char **)kp->arg;
> 	*(char **)kp->arg = tmp;
> 	maybe_kfree_parameter(old);
> 
>>  
>>  	return 0;
>>  }
>