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

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

[Pengpeng Hou]

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.

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)
-- 
2.50.1 (Apple Git-155)

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

[Petr Pavlu]

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)

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

[Pengpeng Hou]

Hi Petr,

Thanks, that makes sense.

I agree that adding an explicit size argument to kernel_param_ops::get()
would be the cleaner interface. My patch was trying to keep the fix
local to param_array_get(), since changing the callback signature would
touch all current implementations.

My inclination would be to keep the immediate fix local to
param_array_get() first, since that seems more suitable for a bug fix and
doesn't require touching every current ->get() implementation.

If you'd rather take the interface cleanup first, I'm happy to work on a
larger series that extends ->get() with a size argument and then uses it
in param_array_get().

Thanks,
Pengpeng

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

[Petr Pavlu]

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.
> 
> Fixes: 9bbb9e5a3310 ("param: use ops in struct kernel_param, rather than get and set fns directly")

I'm not sure how this commit is relevant. It looks to me the issue was
introduced pre-Git by "[PATCH] module parameter array fixes":

https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/?id=206a70f22b5fc94e58a7e75f1d4bce1215c24ad7

> Cc: stable@vger.kernel.org
> 
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>

As mentioned in my previous reply, I think it would be good to look into
making kernel_param_ops::get() take a size argument as well. However,
this patch looks reasonable to me as a minimal fix. Feel free to add:

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

-- 
Thanks,
Petr