kernel/params.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)
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)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
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
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)
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
© 2016 - 2026 Red Hat, Inc.