LKML Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] drm/i915: Fix "mitigations" parsing if i915 is builtin
@ 2021-04-13  9:02 Jisheng Zhang
  2021-04-13 16:59 ` Ville Syrjälä
  0 siblings, 1 reply; 4+ messages in thread
From: Jisheng Zhang @ 2021-04-13  9:02 UTC (permalink / raw)
  To: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, David Airlie,
	Daniel Vetter, Chris Wilson, Jon Bloomfield
  Cc: intel-gfx, dri-devel, linux-kernel

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);
 
 	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;
 
-- 
2.31.0


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] drm/i915: Fix "mitigations" parsing if i915 is builtin
  2021-04-13  9:02 [PATCH] drm/i915: Fix "mitigations" parsing if i915 is builtin Jisheng Zhang
@ 2021-04-13 16:59 ` Ville Syrjälä
  2021-04-14  6:16   ` Jisheng Zhang
  2021-04-14  6:40   ` Jisheng Zhang
  0 siblings, 2 replies; 4+ messages in thread
From: Ville Syrjälä @ 2021-04-13 16:59 UTC (permalink / raw)
  To: Jisheng Zhang
  Cc: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, David Airlie,
	Daniel Vetter, Chris Wilson, Jon Bloomfield, intel-gfx,
	linux-kernel, dri-devel

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.

-- 
Ville Syrjälä
Intel

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] drm/i915: Fix "mitigations" parsing if i915 is builtin
  2021-04-13 16:59 ` Ville Syrjälä
@ 2021-04-14  6:16   ` Jisheng Zhang
  2021-04-14  6:40   ` Jisheng Zhang
  1 sibling, 0 replies; 4+ messages in thread
From: Jisheng Zhang @ 2021-04-14  6:16 UTC (permalink / raw)
  To: Ville Syrjälä
  Cc: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, David Airlie,
	Daniel Vetter, Chris Wilson, Jon Bloomfield, intel-gfx,
	linux-kernel, dri-devel

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] drm/i915: Fix "mitigations" parsing if i915 is builtin
  2021-04-13 16:59 ` Ville Syrjälä
  2021-04-14  6:16   ` Jisheng Zhang
@ 2021-04-14  6:40   ` Jisheng Zhang
  1 sibling, 0 replies; 4+ messages in thread
From: Jisheng Zhang @ 2021-04-14  6:40 UTC (permalink / raw)
  To: Ville Syrjälä
  Cc: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, David Airlie,
	Daniel Vetter, Chris Wilson, Jon Bloomfield, intel-gfx,
	linux-kernel, dri-devel

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-13  9:02 [PATCH] drm/i915: Fix "mitigations" parsing if i915 is builtin Jisheng Zhang
2021-04-13 16:59 ` Ville Syrjälä
2021-04-14  6:16   ` Jisheng Zhang
2021-04-14  6:40   ` Jisheng Zhang

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git
	git clone --mirror https://lore.kernel.org/lkml/10 lkml/git/10.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git