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.

Cc: stable@vger.kernel.org
Reviewed-by: Petr Pavlu <petr.pavlu@suse.com>
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
Changes since v1:
- drop the incorrect Fixes tag; as Petr pointed out, the issue appears
  to predate mainline git history
- add Petr's Reviewed-by
- avoid rewriting the previous separator if the page buffer has no room
  to copy any bytes from the next element
- keep the broader kernel_param_ops::get(buffer, size) conversion as
  follow-up work, leaving this as the minimal fix

 kernel/params.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 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,36 @@ 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));
+		if (!ret)
+			break;
+		/* Replace the previous element's trailing newline with a comma. */
+		if (i)
+			buffer[off - 1] = ',';
+		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)

On 5/7/26 10:21 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.
> 
> Cc: stable@vger.kernel.org
> Reviewed-by: Petr Pavlu <petr.pavlu@suse.com>
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
> Changes since v1:
> - drop the incorrect Fixes tag; as Petr pointed out, the issue appears
>   to predate mainline git history
> - add Petr's Reviewed-by
> - avoid rewriting the previous separator if the page buffer has no room
>   to copy any bytes from the next element

I'm confused by this change. Didn't the simpler v1 already have this
behavior?

-- 
Thanks,
Petr

Hi Petr,

You're right, that changelog bullet was misleading.

v1 already broke out of the loop once off reached PAGE_SIZE - 1, so it
would not enter another iteration with no remaining byte in the caller's
page buffer.

The v2 change was narrower: after the element getter returns, it clamps
the number of bytes to copy and only rewrites the previous '\n' separator
when that clamped length is non-zero. That avoids turning the previous
separator into ',' when the next element contributes no visible bytes
after clamping, or if a getter returns 0.

The bullet should have said:

- avoid rewriting the previous separator when no bytes are copied from
  the next element

The code change itself still matches that behavior. I can resend with
the changelog corrected if preferred.

Thanks,
Pengpeng

On 5/21/26 4:28 AM, Pengpeng Hou wrote:
> Hi Petr,
> 
> You're right, that changelog bullet was misleading.
> 
> v1 already broke out of the loop once off reached PAGE_SIZE - 1, so it
> would not enter another iteration with no remaining byte in the caller's
> page buffer.
> 
> The v2 change was narrower: after the element getter returns, it clamps
> the number of bytes to copy and only rewrites the previous '\n' separator
> when that clamped length is non-zero. That avoids turning the previous
> separator into ',' when the next element contributes no visible bytes
> after clamping, or if a getter returns 0.

The updated code in v2 looks as follows:

	for (i = off = 0; i < (arr->num ? *arr->num : arr->max); i++) {
		p.arg = arr->elem + arr->elemsize * i;
		check_kparam_locked(p.mod);
		ret = arr->ops->get(elem_buf, &p);
		if (ret < 0)
			goto out;
		ret = min(ret, (int)(PAGE_SIZE - 1 - off));
		if (!ret)
			break;
		/* Replace the previous element's trailing newline with a comma. */
		if (i)
			buffer[off - 1] = ',';
		memcpy(buffer + off, elem_buf, ret);
		off += ret;
		if (off == PAGE_SIZE - 1)
			break;
	}

The clamping is done by:

ret = min(ret, (int)(PAGE_SIZE - 1 - off));

My understanding is that the expression '(int)(PAGE_SIZE - 1 - off)'
cannot return 0 because otherwise the loop would have already broken out
in the previous iteration due to the final check
'if (off == PAGE_SIZE - 1)'.

The input ret value to the min() calculation comes from the
arr->ops->get() call. The kernel_param_ops::get() API requires the
resulting string to be terminated by '\n', so on success the call should
never return 0. Even if it does and we want to make param_array_get()
tighter, I believe it should be treated as an error rather than silently
returning success from this function.

-- 
Thanks,
Petr

On Thu, May 07, 2026 at 04:21:03PM +0800, 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.
> 
> Cc: stable@vger.kernel.org
> Reviewed-by: Petr Pavlu <petr.pavlu@suse.com>
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>

Yeah, this is a good first step. I'd really like to change all these
get/set ops here and for sysfs, etc, to use seq_buf, but that's a much
larger change. In the meantime, let's do this.

Reviewed-by: Kees Cook <kees@kernel.org>

-Kees

-- 
Kees Cook