Re: [PATCH] params: bound array element output to the caller's page buffer

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

On 4/17/26 9:50 AM, Pengpeng Hou wrote:
> param_array_get() appends each element's string representation into the
> shared sysfs page buffer by passing buffer + off to the element getter.
> 
> That works for getters that only write a small bounded string, but
> param_get_charp() and similar helpers format against PAGE_SIZE from the
> pointer they receive. Once off is non-zero, an element getter can
> therefore write past the end of the original sysfs page buffer.
> 
> Collect each element into a temporary PAGE_SIZE buffer first and then
> copy only the remaining space into the caller's page buffer.

The underlying issue is that the kernel_param_ops::get() callback only
takes a pointer to a buffer where the result should be stored, with the
implicit knowledge that it is at least PAGE_SIZE in size. The params
code apparently borrows this from the sysfs code, which is
understandable because only sysfs can currently print module parameters.

Nonetheless, the question is whether it would be better to rework the
kernel_param_ops::get() callback to also include a size argument. This
modification would prevent the copying in param_array_get() and having
an explicit size is generally a better interface. It could also be
useful for Rust integration, even though the current code doesn't
support reading module parameters via sysfs. However, this change would
require more work to update all current implementations of this
callback.

-- 
Thanks,
Petr

> 
> Fixes: 9bbb9e5a3310 ("param: use ops in struct kernel_param, rather than get and set fns directly")
> Cc: stable@vger.kernel.org
> 
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
>  kernel/params.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/params.c b/kernel/params.c
> index 74d620bc2521..8910daa12816 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -475,22 +475,34 @@ static int param_array_set(const char *val, const struct kernel_param *kp)
>  static int param_array_get(char *buffer, const struct kernel_param *kp)
>  {
>  	int i, off, ret;
> +	char *elem_buf;
>  	const struct kparam_array *arr = kp->arr;
>  	struct kernel_param p = *kp;
>  
> +	elem_buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
> +	if (!elem_buf)
> +		return -ENOMEM;
> +
>  	for (i = off = 0; i < (arr->num ? *arr->num : arr->max); i++) {
>  		/* Replace \n with comma */
>  		if (i)
>  			buffer[off - 1] = ',';
>  		p.arg = arr->elem + arr->elemsize * i;
>  		check_kparam_locked(p.mod);
> -		ret = arr->ops->get(buffer + off, &p);
> +		ret = arr->ops->get(elem_buf, &p);
>  		if (ret < 0)
> -			return ret;
> +			goto out;
> +		ret = min(ret, (int)(PAGE_SIZE - 1 - off));
> +		memcpy(buffer + off, elem_buf, ret);
>  		off += ret;
> +		if (off == PAGE_SIZE - 1)
> +			break;
>  	}
>  	buffer[off] = '\0';
> -	return off;
> +	ret = off;
> +out:
> +	kfree(elem_buf);
> +	return ret;
>  }
>  
>  static void param_array_free(void *arg)