| Message ID | 20210413170240.0d4ffa38@xhacker.debian (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Fix "mitigations" parsing if i915 is builtin | expand |
On Tue, Apr 13, 2021 at 05:02:40PM +0800, Jisheng Zhang wrote: > I met below error during boot with i915 builtin if pass > "i915.mitigations=off": > [ 0.015589] Booting kernel: `off' invalid for parameter `i915.mitigations' > > The reason is slab subsystem isn't ready at that time, so kstrdup() > returns NULL. Fix this issue by using stack var instead of kstrdup(). > > Fixes: 984cadea032b ("drm/i915: Allow the sysadmin to override security mitigations") > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com> > --- > drivers/gpu/drm/i915/i915_mitigations.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_mitigations.c b/drivers/gpu/drm/i915/i915_mitigations.c > index 84f12598d145..7dadf41064e0 100644 > --- a/drivers/gpu/drm/i915/i915_mitigations.c > +++ b/drivers/gpu/drm/i915/i915_mitigations.c > @@ -29,15 +29,13 @@ bool i915_mitigate_clear_residuals(void) > static int mitigations_set(const char *val, const struct kernel_param *kp) > { > unsigned long new = ~0UL; > - char *str, *sep, *tok; > + char str[64], *sep, *tok; > bool first = true; > int err = 0; > > BUILD_BUG_ON(ARRAY_SIZE(names) >= BITS_PER_TYPE(mitigations)); > > - str = kstrdup(val, GFP_KERNEL); > - if (!str) > - return -ENOMEM; > + strncpy(str, val, sizeof(str) - 1); I don't think strncpy() guarantees that the string is properly terminated. Also commit b1b6bed3b503 ("usb: core: fix quirks_param_set() writing to a const pointer") looks broken as well given your findings, and arch/um/drivers/virtio_uml.c seems to suffer from this as well. kernel/params.c itself seems to have some slab_is_available() magic around kmalloc(). I used the following cocci snippet to find these: @find@ identifier O, F; position PS; @@ struct kernel_param_ops O = { ..., .set = F@PS ,... }; @alloc@ identifier ALLOC =~ "^k.*(alloc|dup)"; identifier find.F; position PA; @@ F(...) { <+... ALLOC@PA(...) ...+> } @script:python depends on alloc@ ps << find.PS; pa << alloc.PA; @@ coccilib.report.print_report(ps[0], "struct") coccilib.report.print_report(pa[0], "alloc") That could of course miss a bunch more if they allocate via some other function I didn't consider.
On Tue, 13 Apr 2021 19:59:34 +0300 Ville Syrjälä wrote: > > On Tue, Apr 13, 2021 at 05:02:40PM +0800, Jisheng Zhang wrote: > > I met below error during boot with i915 builtin if pass > > "i915.mitigations=off": > > [ 0.015589] Booting kernel: `off' invalid for parameter `i915.mitigations' > > > > The reason is slab subsystem isn't ready at that time, so kstrdup() > > returns NULL. Fix this issue by using stack var instead of kstrdup(). > > > > Fixes: 984cadea032b ("drm/i915: Allow the sysadmin to override security mitigations") > > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com> > > --- > > drivers/gpu/drm/i915/i915_mitigations.c | 7 ++----- > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_mitigations.c b/drivers/gpu/drm/i915/i915_mitigations.c > > index 84f12598d145..7dadf41064e0 100644 > > --- a/drivers/gpu/drm/i915/i915_mitigations.c > > +++ b/drivers/gpu/drm/i915/i915_mitigations.c > > @@ -29,15 +29,13 @@ bool i915_mitigate_clear_residuals(void) > > static int mitigations_set(const char *val, const struct kernel_param *kp) > > { > > unsigned long new = ~0UL; > > - char *str, *sep, *tok; > > + char str[64], *sep, *tok; > > bool first = true; > > int err = 0; > > > > BUILD_BUG_ON(ARRAY_SIZE(names) >= BITS_PER_TYPE(mitigations)); > > > > - str = kstrdup(val, GFP_KERNEL); > > - if (!str) > > - return -ENOMEM; > > + strncpy(str, val, sizeof(str) - 1); > > I don't think strncpy() guarantees that the string is properly > terminated. > > Also commit b1b6bed3b503 ("usb: core: fix quirks_param_set() writing to > a const pointer") looks broken as well given your findings, and > arch/um/drivers/virtio_uml.c seems to suffer from this as well. wow thank you so much. I will send out patches to fix them as well. > kernel/params.c itself seems to have some slab_is_available() magic > around kmalloc(). > > I used the following cocci snippet to find these: Nice cocci script. > @find@ > identifier O, F; > position PS; > @@ > struct kernel_param_ops O = { > ..., > .set = F@PS > ,... > }; > > @alloc@ > identifier ALLOC =~ "^k.*(alloc|dup)"; > identifier find.F; > position PA; > @@ > F(...) { > <+... > ALLOC@PA(...) > ...+> > } > > @script:python depends on alloc@ > ps << find.PS; > pa << alloc.PA; > @@ > coccilib.report.print_report(ps[0], "struct") > coccilib.report.print_report(pa[0], "alloc") > > That could of course miss a bunch more if they allocate > via some other function I didn't consider. > > -- > Ville Syrjälä > Intel
Hi Ville, On Tue, 13 Apr 2021 19:59:34 +0300 Ville Syrjälä wrote: > > > On Tue, Apr 13, 2021 at 05:02:40PM +0800, Jisheng Zhang wrote: > > I met below error during boot with i915 builtin if pass > > "i915.mitigations=off": > > [ 0.015589] Booting kernel: `off' invalid for parameter `i915.mitigations' > > > > The reason is slab subsystem isn't ready at that time, so kstrdup() > > returns NULL. Fix this issue by using stack var instead of kstrdup(). > > > > Fixes: 984cadea032b ("drm/i915: Allow the sysadmin to override security mitigations") > > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com> > > --- > > drivers/gpu/drm/i915/i915_mitigations.c | 7 ++----- > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_mitigations.c b/drivers/gpu/drm/i915/i915_mitigations.c > > index 84f12598d145..7dadf41064e0 100644 > > --- a/drivers/gpu/drm/i915/i915_mitigations.c > > +++ b/drivers/gpu/drm/i915/i915_mitigations.c > > @@ -29,15 +29,13 @@ bool i915_mitigate_clear_residuals(void) > > static int mitigations_set(const char *val, const struct kernel_param *kp) > > { > > unsigned long new = ~0UL; > > - char *str, *sep, *tok; > > + char str[64], *sep, *tok; > > bool first = true; > > int err = 0; > > > > BUILD_BUG_ON(ARRAY_SIZE(names) >= BITS_PER_TYPE(mitigations)); > > > > - str = kstrdup(val, GFP_KERNEL); > > - if (!str) > > - return -ENOMEM; > > + strncpy(str, val, sizeof(str) - 1); > > I don't think strncpy() guarantees that the string is properly > terminated. > > Also commit b1b6bed3b503 ("usb: core: fix quirks_param_set() writing to > a const pointer") looks broken as well given your findings, and > arch/um/drivers/virtio_uml.c seems to suffer from this as well. > kernel/params.c itself seems to have some slab_is_available() magic > around kmalloc(). Just tried the "usbcore.quirks" with usb builtin, I can't reproduce the issue. Futher investigation shows that device_param_cb() macro is the key, or the "6" in __level_param_cb(name, ops, arg, perm, 6) is the key. While i915.mitigations uses module_param_cb_unsafe(), in which the level will be "-1" arch/um/drivers/virtio_uml.c also makes use of device_param_cb() thanks
diff --git a/drivers/gpu/drm/i915/i915_mitigations.c b/drivers/gpu/drm/i915/i915_mitigations.c index 84f12598d145..7dadf41064e0 100644 --- a/drivers/gpu/drm/i915/i915_mitigations.c +++ b/drivers/gpu/drm/i915/i915_mitigations.c @@ -29,15 +29,13 @@ bool i915_mitigate_clear_residuals(void) static int mitigations_set(const char *val, const struct kernel_param *kp) { unsigned long new = ~0UL; - char *str, *sep, *tok; + char str[64], *sep, *tok; bool first = true; int err = 0; BUILD_BUG_ON(ARRAY_SIZE(names) >= BITS_PER_TYPE(mitigations)); - str = kstrdup(val, GFP_KERNEL); - if (!str) - return -ENOMEM; + strncpy(str, val, sizeof(str) - 1); for (sep = str; (tok = strsep(&sep, ","));) { bool enable = true; @@ -86,7 +84,6 @@ static int mitigations_set(const char *val, const struct kernel_param *kp) break; } } - kfree(str); if (err) return err;
I met below error during boot with i915 builtin if pass "i915.mitigations=off": [ 0.015589] Booting kernel: `off' invalid for parameter `i915.mitigations' The reason is slab subsystem isn't ready at that time, so kstrdup() returns NULL. Fix this issue by using stack var instead of kstrdup(). Fixes: 984cadea032b ("drm/i915: Allow the sysadmin to override security mitigations") Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com> --- drivers/gpu/drm/i915/i915_mitigations.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)