Thank you for your contribution! Sashiko AI review found 1 potential issue(s) 
to consider:

Pre-existing issues:
- [High] Unconditional call to `arr->ops->get()` without a NULL check can cause 
a kernel panic if the array parameter element lacks a `.get` callback.
--

commit a252ae87a187a5b644d160fded352c545493a611
Author: Kees Cook <[email protected]>

moduleparam: Drop legacy kernel_param_ops .get_str field and dispatch logic

All struct kernel_param_ops .get callbacks have been migrated to using
struct seq_buf. Drop the migration scaffolding.

> diff --git a/kernel/params.c b/kernel/params.c
> index 25f0c8d5d19f3..6b410189297b1 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
[ ... ]
> @@ -470,23 +469,9 @@ static int param_array_get(struct seq_buf *s, const 
> struct kernel_param *kp)
>               p.arg = arr->elem + arr->elemsize * i;
>               check_kparam_locked(p.mod);
>  
> -             if (arr->ops->get) {
> -                     ret = arr->ops->get(s, &p);
> -                     if (ret < 0)
> -                             goto out;
> -             } else {
[ ... ]
> -             }
> +             ret = arr->ops->get(s, &p);

[Severity: High]
This isn't a bug introduced by this patch, but does this unconditional call to
arr->ops->get() need a NULL check to avoid a kernel panic?

If a module defines a write-only array parameter where the element's
kernel_param_ops struct lacks a .get callback, and assigns it write-only sysfs
permissions, root can still attempt to read the file. When this happens, sysfs
invokes param_attr_show().

For normal parameters, param_attr_show() has a safety check that rejects the
read if ops->get is NULL. However, for array parameters, ops resolves to
param_array_ops which has a valid .get callback, bypassing the safeguard.

Could this result in a NULL pointer dereference when arr->ops->get(s, &p) is
executed? Before this patch, the code fell back to calling .get_str(), which
would also be NULL for a write-only type, resulting in the same kernel panic.

> +             if (ret < 0)
> +                     return ret;
>  
>               /* Nothing got written (e.g. overflow) — stop. */
>               if (s->len == before)

-- 
Sashiko AI review · 
https://sashiko.dev/#/patchset/[email protected]?part=11

Reply via email to