* [PATCH] of: do not leak console options
@ 2017-08-25 17:36 Sergey Senozhatsky
2017-08-25 22:37 ` Rob Herring
0 siblings, 1 reply; 14+ messages in thread
From: Sergey Senozhatsky @ 2017-08-25 17:36 UTC (permalink / raw)
To: Rob Herring
Cc: Petr Mladek, Steven Rostedt, devicetree, linux-kernel,
Sergey Senozhatsky
If add_preferred_console() returns error then we must free a
copy of `of_stdout_options' that we create right before the
console registration.
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
drivers/of/base.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 260d33c0f26c..9fcf7011d206 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1779,10 +1779,17 @@ EXPORT_SYMBOL_GPL(of_alias_get_highest_id);
*/
bool of_console_check(struct device_node *dn, char *name, int index)
{
+ bool ret;
+ char *options;
+
if (!dn || dn != of_stdout || console_set_on_cmdline)
return false;
- return !add_preferred_console(name, index,
- kstrdup(of_stdout_options, GFP_KERNEL));
+
+ options = kstrdup(of_stdout_options, GFP_KERNEL);
+ ret = add_preferred_console(name, index, options);
+ if (ret)
+ kfree(options);
+ return !ret;
}
EXPORT_SYMBOL_GPL(of_console_check);
--
2.14.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] of: do not leak console options
2017-08-25 17:36 [PATCH] of: do not leak console options Sergey Senozhatsky
@ 2017-08-25 22:37 ` Rob Herring
2017-08-27 7:19 ` Sergey Senozhatsky
0 siblings, 1 reply; 14+ messages in thread
From: Rob Herring @ 2017-08-25 22:37 UTC (permalink / raw)
To: Sergey Senozhatsky; +Cc: Petr Mladek, Steven Rostedt, devicetree, linux-kernel
On Sat, Aug 26, 2017 at 02:36:47AM +0900, Sergey Senozhatsky wrote:
> If add_preferred_console() returns error then we must free a
> copy of `of_stdout_options' that we create right before the
> console registration.
>
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
> drivers/of/base.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 260d33c0f26c..9fcf7011d206 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1779,10 +1779,17 @@ EXPORT_SYMBOL_GPL(of_alias_get_highest_id);
> */
> bool of_console_check(struct device_node *dn, char *name, int index)
> {
> + bool ret;
> + char *options;
> +
> if (!dn || dn != of_stdout || console_set_on_cmdline)
> return false;
> - return !add_preferred_console(name, index,
> - kstrdup(of_stdout_options, GFP_KERNEL));
> +
> + options = kstrdup(of_stdout_options, GFP_KERNEL);
The real question is why are we doing the kstrdup in the first place.
AFAICT, the only reason is options within the console/printk code is a
char * and not a const char *. I can't imagine that options need
modifications?
Rob
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] of: do not leak console options
2017-08-25 22:37 ` Rob Herring
@ 2017-08-27 7:19 ` Sergey Senozhatsky
2017-08-27 8:01 ` Sergey Senozhatsky
2017-09-06 12:40 ` Petr Mladek
0 siblings, 2 replies; 14+ messages in thread
From: Sergey Senozhatsky @ 2017-08-27 7:19 UTC (permalink / raw)
To: Rob Herring
Cc: Sergey Senozhatsky, Petr Mladek, Steven Rostedt, devicetree,
linux-kernel, Andrew Lunn, Grant Likely
Hello,
(Cc Andrew, Grant)
On (08/25/17 17:37), Rob Herring wrote:
> On Sat, Aug 26, 2017 at 02:36:47AM +0900, Sergey Senozhatsky wrote:
> > If add_preferred_console() returns error then we must free a
> > copy of `of_stdout_options' that we create right before the
> > console registration.
> >
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > ---
> > drivers/of/base.c | 11 +++++++++--
> > 1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index 260d33c0f26c..9fcf7011d206 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -1779,10 +1779,17 @@ EXPORT_SYMBOL_GPL(of_alias_get_highest_id);
> > */
> > bool of_console_check(struct device_node *dn, char *name, int index)
> > {
> > + bool ret;
> > + char *options;
> > +
> > if (!dn || dn != of_stdout || console_set_on_cmdline)
> > return false;
> > - return !add_preferred_console(name, index,
> > - kstrdup(of_stdout_options, GFP_KERNEL));
> > +
> > + options = kstrdup(of_stdout_options, GFP_KERNEL);
>
> The real question is why are we doing the kstrdup in the first place.
I wouldn't know :) let's find that out
the patch used to pass `of_stdout_options' in v1 and v2
https://patches.linaro.org/patch/41559/
starting from v3 options are kstrdup-ed
https://patchwork.kernel.org/patch/5398761/
> AFAICT, the only reason is options within the console/printk code is a
> char * and not a const char *. I can't imagine that options need
> modifications?
as far as I can tell, ->match callback has side efects, sometimes.
for example,
register_console()
newcon->match(newcon, c->name, c->index, c->options)
for amba-pl011.c ends up in pl011_console_match(), which calls
uart_parse_earlycon(options, &iotype, &addr, &options)
and uart_parse_earlycon() does the following
int uart_parse_earlycon(char *p, unsigned char *iotype, resource_size_t *addr,
char **options)
{
if (strncmp(p, "mmio,", 5) == 0) {
*iotype = UPIO_MEM;
p += 5;
} else if (strncmp(p, "mmio16,", 7) == 0) {
*iotype = UPIO_MEM16;
p += 7;
} else if (strncmp(p, "mmio32,", 7) == 0) {
*iotype = UPIO_MEM32;
p += 7;
} else if (strncmp(p, "mmio32be,", 9) == 0) {
*iotype = UPIO_MEM32BE;
p += 9;
} else if (strncmp(p, "mmio32native,", 13) == 0) {
*iotype = IS_ENABLED(CONFIG_CPU_BIG_ENDIAN) ?
UPIO_MEM32BE : UPIO_MEM32;
p += 13;
} else if (strncmp(p, "io,", 3) == 0) {
*iotype = UPIO_PORT;
p += 3;
} else if (strncmp(p, "0x", 2) == 0) {
*iotype = UPIO_MEM;
} else {
return -EINVAL;
}
/*
* Before you replace it with kstrtoull(), think about options separator
* (',') it will not tolerate
*/
*addr = simple_strtoull(p, NULL, 0);
p = strchr(p, ',');
if (p)
p++;
*options = p;
return 0;
}
that's just one example I found after a very quick grepping. may be
there are other control paths that can change ->options.
wondering if something like this was the reason for kstrdup().
-ss
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] of: do not leak console options
2017-08-27 7:19 ` Sergey Senozhatsky
@ 2017-08-27 8:01 ` Sergey Senozhatsky
2017-09-06 12:40 ` Petr Mladek
1 sibling, 0 replies; 14+ messages in thread
From: Sergey Senozhatsky @ 2017-08-27 8:01 UTC (permalink / raw)
To: Rob Herring
Cc: Petr Mladek, Steven Rostedt, devicetree, linux-kernel,
Andrew Lunn, Grant Likely, Sergey Senozhatsky
On (08/27/17 16:19), Sergey Senozhatsky wrote:
[..]
> int uart_parse_earlycon(char *p, unsigned char *iotype, resource_size_t *addr,
> char **options)
> {
> if (strncmp(p, "mmio,", 5) == 0) {
> *iotype = UPIO_MEM;
> p += 5;
> } else if (strncmp(p, "mmio16,", 7) == 0) {
> *iotype = UPIO_MEM16;
> p += 7;
> } else if (strncmp(p, "mmio32,", 7) == 0) {
> *iotype = UPIO_MEM32;
> p += 7;
> } else if (strncmp(p, "mmio32be,", 9) == 0) {
> *iotype = UPIO_MEM32BE;
> p += 9;
> } else if (strncmp(p, "mmio32native,", 13) == 0) {
> *iotype = IS_ENABLED(CONFIG_CPU_BIG_ENDIAN) ?
> UPIO_MEM32BE : UPIO_MEM32;
> p += 13;
> } else if (strncmp(p, "io,", 3) == 0) {
> *iotype = UPIO_PORT;
> p += 3;
> } else if (strncmp(p, "0x", 2) == 0) {
> *iotype = UPIO_MEM;
> } else {
> return -EINVAL;
> }
>
> /*
> * Before you replace it with kstrtoull(), think about options separator
> * (',') it will not tolerate
> */
> *addr = simple_strtoull(p, NULL, 0);
> p = strchr(p, ',');
> if (p)
> p++;
>
> *options = p;
> return 0;
> }
>
> that's just one example I found after a very quick grepping. may be
> there are other control paths that can change ->options.
well, obviously, it doesn't change the options per se, but it sort of
demonstrates that _probably_ somewhere ->options could be changed. on
the other hand, that ->options = ->options + X makes it rather hard
to kfree() the ->options. well, we never kfree() the ->options anyway,
as far as I can tell...
-ss
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] of: do not leak console options
2017-08-27 7:19 ` Sergey Senozhatsky
2017-08-27 8:01 ` Sergey Senozhatsky
@ 2017-09-06 12:40 ` Petr Mladek
2017-09-06 13:29 ` Sergey Senozhatsky
1 sibling, 1 reply; 14+ messages in thread
From: Petr Mladek @ 2017-09-06 12:40 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Rob Herring, Steven Rostedt, devicetree, linux-kernel,
Andrew Lunn, Grant Likely
On Sun 2017-08-27 16:19:10, Sergey Senozhatsky wrote:
> Hello,
>
> (Cc Andrew, Grant)
>
> On (08/25/17 17:37), Rob Herring wrote:
> > On Sat, Aug 26, 2017 at 02:36:47AM +0900, Sergey Senozhatsky wrote:
> > > If add_preferred_console() returns error then we must free a
> > > copy of `of_stdout_options' that we create right before the
> > > console registration.
> > >
> > > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > > ---
> > > drivers/of/base.c | 11 +++++++++--
> > > 1 file changed, 9 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > > index 260d33c0f26c..9fcf7011d206 100644
> > > --- a/drivers/of/base.c
> > > +++ b/drivers/of/base.c
> > > @@ -1779,10 +1779,17 @@ EXPORT_SYMBOL_GPL(of_alias_get_highest_id);
> > > */
> > > bool of_console_check(struct device_node *dn, char *name, int index)
> > > {
> > > + bool ret;
> > > + char *options;
> > > +
> > > if (!dn || dn != of_stdout || console_set_on_cmdline)
> > > return false;
> > > - return !add_preferred_console(name, index,
> > > - kstrdup(of_stdout_options, GFP_KERNEL));
> > > +
> > > + options = kstrdup(of_stdout_options, GFP_KERNEL);
> >
> > The real question is why are we doing the kstrdup in the first place.
>
> I wouldn't know :) let's find that out
>
> the patch used to pass `of_stdout_options' in v1 and v2
> https://patches.linaro.org/patch/41559/
>
> starting from v3 options are kstrdup-ed
> https://patchwork.kernel.org/patch/5398761/
I was curious. The const char * was suggested by Grant Likely,
see https://lkml.kernel.org/r/CACxGe6tQ5rWzCUcS+_fFY+rjEyua2khApAoCVKpTuJAghU=N_w@mail.gmail.com
I guess that the reason was to make the of_find_node_by_path()
API clean.
> > AFAICT, the only reason is options within the console/printk code is a
> > char * and not a const char *. I can't imagine that options need
> > modifications?
>
> as far as I can tell, ->match callback has side efects, sometimes.
I hope that the match() callbacks does not have this kind of side
effects. I think that they initialize some stuff, assign some values.
But I hope that they do not modify given strings, like console
name or options. At leats I am unable to find any place.
But I am not 100% sure.
Sigh, the console code really needs clean up.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] of: do not leak console options
2017-09-06 12:40 ` Petr Mladek
@ 2017-09-06 13:29 ` Sergey Senozhatsky
2017-09-06 15:27 ` Rob Herring
0 siblings, 1 reply; 14+ messages in thread
From: Sergey Senozhatsky @ 2017-09-06 13:29 UTC (permalink / raw)
To: Petr Mladek, Rob Herring, Grant Likely
Cc: Sergey Senozhatsky, Steven Rostedt, devicetree, linux-kernel,
Andrew Lunn
Hello,
On (09/06/17 14:40), Petr Mladek wrote:
[..]
> > I wouldn't know :) let's find that out
> >
> > the patch used to pass `of_stdout_options' in v1 and v2
> > https://patches.linaro.org/patch/41559/
> >
> > starting from v3 options are kstrdup-ed
> > https://patchwork.kernel.org/patch/5398761/
>
> I was curious. The const char * was suggested by Grant Likely,
> see https://lkml.kernel.org/r/CACxGe6tQ5rWzCUcS+_fFY+rjEyua2khApAoCVKpTuJAghU=N_w@mail.gmail.com
> I guess that the reason was to make the of_find_node_by_path()
> API clean.
ok, thanks.
> > > AFAICT, the only reason is options within the console/printk code is a
> > > char * and not a const char *. I can't imagine that options need
> > > modifications?
> >
> > as far as I can tell, ->match callback has side efects, sometimes.
>
> I hope that the match() callbacks does not have this kind of side
> effects. I think that they initialize some stuff, assign some values.
> But I hope that they do not modify given strings, like console
> name or options. At leats I am unable to find any place.
> But I am not 100% sure.
yeah, seems like we can pass just char *options.
const-ifying options (and brl options) on the printk side would probably
be a better solution. need to check if we can do that, tho. that also
would require touching printk API, struct console, struct console_cmdline,
->match callbacks of every console in the kernel, etc. not like a big deal,
just potentially a bit of a noisy patch.
Rob, Grant,
will this dirty hack work for you? I can respin the patch.
---
drivers/of/base.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 260d33c0f26c..e6839045c454 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1781,8 +1781,8 @@ bool of_console_check(struct device_node *dn, char *name, int index)
{
if (!dn || dn != of_stdout || console_set_on_cmdline)
return false;
- return !add_preferred_console(name, index,
- kstrdup(of_stdout_options, GFP_KERNEL));
+
+ return !add_preferred_console(name, index, (char *)of_stdout_options);
}
EXPORT_SYMBOL_GPL(of_console_check);
--
2.14.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] of: do not leak console options
2017-09-06 13:29 ` Sergey Senozhatsky
@ 2017-09-06 15:27 ` Rob Herring
2017-09-07 9:57 ` Sergey Senozhatsky
2017-09-26 6:31 ` Sergey Senozhatsky
0 siblings, 2 replies; 14+ messages in thread
From: Rob Herring @ 2017-09-06 15:27 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Petr Mladek, Grant Likely, Sergey Senozhatsky, Steven Rostedt,
devicetree, linux-kernel, Andrew Lunn
On Wed, Sep 6, 2017 at 8:29 AM, Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:
> Hello,
>
> On (09/06/17 14:40), Petr Mladek wrote:
> [..]
>> > I wouldn't know :) let's find that out
>> >
>> > the patch used to pass `of_stdout_options' in v1 and v2
>> > https://patches.linaro.org/patch/41559/
>> >
>> > starting from v3 options are kstrdup-ed
>> > https://patchwork.kernel.org/patch/5398761/
>>
>> I was curious. The const char * was suggested by Grant Likely,
>> see https://lkml.kernel.org/r/CACxGe6tQ5rWzCUcS+_fFY+rjEyua2khApAoCVKpTuJAghU=N_w@mail.gmail.com
>> I guess that the reason was to make the of_find_node_by_path()
>> API clean.
>
> ok, thanks.
>
>> > > AFAICT, the only reason is options within the console/printk code is a
>> > > char * and not a const char *. I can't imagine that options need
>> > > modifications?
>> >
>> > as far as I can tell, ->match callback has side efects, sometimes.
>>
>> I hope that the match() callbacks does not have this kind of side
>> effects. I think that they initialize some stuff, assign some values.
>> But I hope that they do not modify given strings, like console
>> name or options. At leats I am unable to find any place.
>> But I am not 100% sure.
>
> yeah, seems like we can pass just char *options.
>
> const-ifying options (and brl options) on the printk side would probably
> be a better solution. need to check if we can do that, tho. that also
> would require touching printk API, struct console, struct console_cmdline,
> ->match callbacks of every console in the kernel, etc. not like a big deal,
> just potentially a bit of a noisy patch.
>
>
> Rob, Grant,
> will this dirty hack work for you? I can respin the patch.
Yes, as long as you intend to fix things later.
Rob
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] of: do not leak console options
2017-09-06 15:27 ` Rob Herring
@ 2017-09-07 9:57 ` Sergey Senozhatsky
2017-09-26 6:31 ` Sergey Senozhatsky
1 sibling, 0 replies; 14+ messages in thread
From: Sergey Senozhatsky @ 2017-09-07 9:57 UTC (permalink / raw)
To: Rob Herring
Cc: Sergey Senozhatsky, Petr Mladek, Grant Likely,
Sergey Senozhatsky, Steven Rostedt, devicetree, linux-kernel,
Andrew Lunn
On (09/06/17 10:27), Rob Herring wrote:
[..]
>
> Yes, as long as you intend to fix things later.
Hello Rob,
I can try later, but can't guarantee that will succeed :)
-ss
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] of: do not leak console options
2017-09-06 15:27 ` Rob Herring
2017-09-07 9:57 ` Sergey Senozhatsky
@ 2017-09-26 6:31 ` Sergey Senozhatsky
1 sibling, 0 replies; 14+ messages in thread
From: Sergey Senozhatsky @ 2017-09-26 6:31 UTC (permalink / raw)
To: Rob Herring
Cc: Sergey Senozhatsky, Petr Mladek, Grant Likely,
Sergey Senozhatsky, Steven Rostedt, devicetree, linux-kernel,
Andrew Lunn
On (09/06/17 10:27), Rob Herring wrote:
[..]
> > Rob, Grant,
> > will this dirty hack work for you? I can respin the patch.
>
> Yes, as long as you intend to fix things later.
Rob, sorry for the delay, I just sent out a simple patch. It contains that
'less-than-pretty' const cast. As of updating printk API -- I took a quick
look... and there are quite a lot things to touch: not just printk.c, but
also UART and all console drivers; we pass options not only to ->match()
callback, but to ->setup() callback as well. I'm not sure I've got enough
spare time for it right now, sorry.
-ss
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] of: do not leak console options
2017-10-13 19:48 ` Geert Uytterhoeven
@ 2017-10-13 20:44 ` Rob Herring
0 siblings, 0 replies; 14+ messages in thread
From: Rob Herring @ 2017-10-13 20:44 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Petr Mladek, Sergey Senozhatsky, Steven Rostedt, devicetree,
linux-kernel, Sergey Senozhatsky
On Fri, Oct 13, 2017 at 2:48 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Tue, Oct 3, 2017 at 9:20 AM, Petr Mladek <pmladek@suse.com> wrote:
>> On Tue 2017-09-26 15:25:10, Sergey Senozhatsky wrote:
>>> Do not strdup() console options. It seems that the only reason for
>>> it to be strdup()-ed was a compilation warning: printk, UART and
>>> console drivers, for some reason, expect char pointer instead of
>>> const char pointer. So we can just pass `of_stdout_options', but
>>> need to cast it to char pointer. A better fix would be to change
>>> printk, console drivers and UART to accept const char `options';
>>> but that will take time - there are lots of drivers to update.
>>>
>>> The patch also fixes a possible memory leak: add_preferred_console()
>>> can fail, but we don't kfree() options.
>>>
>>> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>>
>> Of course, it would be better to change add_preferred_console().
>> But it would trigger many other changes. This alternative
>> "temporary" change looks safe to me. Feel free to use
>
> Really? Unless I'm missing something (it's been way too many hours after my
> morning coffee), add_preferred_console() just calls __add_preferred_console(),
> and the latter doesn't touch the passed string. As passing a "char *" to a
> function that excepts a "const char *" is fine, you can just add "const" to
> both add_preferred_console() and __add_preferred_console(), and be done with
> it.
You've only moved the cast down one level. See the prior
discussion[1]. It gets passed to match and setup functions of the
console drivers none of which actually modify the string, but it's
just a lot of drivers to go fix. Maybe someone wants to write a
coccinelle patch.
Anyway, it's in Linus' tree now.
Rob
[1] https://lkml.org/lkml/2017/8/25/784
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] of: do not leak console options
2017-10-03 7:20 ` Petr Mladek
@ 2017-10-13 19:48 ` Geert Uytterhoeven
2017-10-13 20:44 ` Rob Herring
0 siblings, 1 reply; 14+ messages in thread
From: Geert Uytterhoeven @ 2017-10-13 19:48 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Rob Herring, Steven Rostedt, devicetree,
linux-kernel, Sergey Senozhatsky
On Tue, Oct 3, 2017 at 9:20 AM, Petr Mladek <pmladek@suse.com> wrote:
> On Tue 2017-09-26 15:25:10, Sergey Senozhatsky wrote:
>> Do not strdup() console options. It seems that the only reason for
>> it to be strdup()-ed was a compilation warning: printk, UART and
>> console drivers, for some reason, expect char pointer instead of
>> const char pointer. So we can just pass `of_stdout_options', but
>> need to cast it to char pointer. A better fix would be to change
>> printk, console drivers and UART to accept const char `options';
>> but that will take time - there are lots of drivers to update.
>>
>> The patch also fixes a possible memory leak: add_preferred_console()
>> can fail, but we don't kfree() options.
>>
>> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>
> Of course, it would be better to change add_preferred_console().
> But it would trigger many other changes. This alternative
> "temporary" change looks safe to me. Feel free to use
Really? Unless I'm missing something (it's been way too many hours after my
morning coffee), add_preferred_console() just calls __add_preferred_console(),
and the latter doesn't touch the passed string. As passing a "char *" to a
function that excepts a "const char *" is fine, you can just add "const" to
both add_preferred_console() and __add_preferred_console(), and be done with
it.
Repeat after me: casts are evil.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] of: do not leak console options
2017-09-26 6:25 Sergey Senozhatsky
2017-10-03 7:20 ` Petr Mladek
@ 2017-10-12 17:24 ` Rob Herring
1 sibling, 0 replies; 14+ messages in thread
From: Rob Herring @ 2017-10-12 17:24 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Petr Mladek, Steven Rostedt, devicetree, linux-kernel,
Sergey Senozhatsky
On Tue, Sep 26, 2017 at 1:25 AM, Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:
> Do not strdup() console options. It seems that the only reason for
> it to be strdup()-ed was a compilation warning: printk, UART and
> console drivers, for some reason, expect char pointer instead of
> const char pointer. So we can just pass `of_stdout_options', but
> need to cast it to char pointer. A better fix would be to change
> printk, console drivers and UART to accept const char `options';
> but that will take time - there are lots of drivers to update.
>
> The patch also fixes a possible memory leak: add_preferred_console()
> can fail, but we don't kfree() options.
>
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
> drivers/of/base.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
Applied.
Rob
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] of: do not leak console options
2017-09-26 6:25 Sergey Senozhatsky
@ 2017-10-03 7:20 ` Petr Mladek
2017-10-13 19:48 ` Geert Uytterhoeven
2017-10-12 17:24 ` Rob Herring
1 sibling, 1 reply; 14+ messages in thread
From: Petr Mladek @ 2017-10-03 7:20 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Rob Herring, Steven Rostedt, devicetree, linux-kernel,
Sergey Senozhatsky
On Tue 2017-09-26 15:25:10, Sergey Senozhatsky wrote:
> Do not strdup() console options. It seems that the only reason for
> it to be strdup()-ed was a compilation warning: printk, UART and
> console drivers, for some reason, expect char pointer instead of
> const char pointer. So we can just pass `of_stdout_options', but
> need to cast it to char pointer. A better fix would be to change
> printk, console drivers and UART to accept const char `options';
> but that will take time - there are lots of drivers to update.
>
> The patch also fixes a possible memory leak: add_preferred_console()
> can fail, but we don't kfree() options.
>
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Of course, it would be better to change add_preferred_console().
But it would trigger many other changes. This alternative
"temporary" change looks safe to me. Feel free to use
Reviewed-by: Petr Mladek <pmladek@suse.com>
Best Regards,
Petr
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] of: do not leak console options
@ 2017-09-26 6:25 Sergey Senozhatsky
2017-10-03 7:20 ` Petr Mladek
2017-10-12 17:24 ` Rob Herring
0 siblings, 2 replies; 14+ messages in thread
From: Sergey Senozhatsky @ 2017-09-26 6:25 UTC (permalink / raw)
To: Rob Herring
Cc: Petr Mladek, Steven Rostedt, devicetree, linux-kernel,
Sergey Senozhatsky
Do not strdup() console options. It seems that the only reason for
it to be strdup()-ed was a compilation warning: printk, UART and
console drivers, for some reason, expect char pointer instead of
const char pointer. So we can just pass `of_stdout_options', but
need to cast it to char pointer. A better fix would be to change
printk, console drivers and UART to accept const char `options';
but that will take time - there are lots of drivers to update.
The patch also fixes a possible memory leak: add_preferred_console()
can fail, but we don't kfree() options.
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
drivers/of/base.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 260d33c0f26c..63897531cd75 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1781,8 +1781,12 @@ bool of_console_check(struct device_node *dn, char *name, int index)
{
if (!dn || dn != of_stdout || console_set_on_cmdline)
return false;
- return !add_preferred_console(name, index,
- kstrdup(of_stdout_options, GFP_KERNEL));
+
+ /*
+ * XXX: cast `options' to char pointer to suppress complication
+ * warnings: printk, UART and console drivers expect char pointer.
+ */
+ return !add_preferred_console(name, index, (char *)of_stdout_options);
}
EXPORT_SYMBOL_GPL(of_console_check);
--
2.14.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2017-10-13 20:44 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-25 17:36 [PATCH] of: do not leak console options Sergey Senozhatsky
2017-08-25 22:37 ` Rob Herring
2017-08-27 7:19 ` Sergey Senozhatsky
2017-08-27 8:01 ` Sergey Senozhatsky
2017-09-06 12:40 ` Petr Mladek
2017-09-06 13:29 ` Sergey Senozhatsky
2017-09-06 15:27 ` Rob Herring
2017-09-07 9:57 ` Sergey Senozhatsky
2017-09-26 6:31 ` Sergey Senozhatsky
2017-09-26 6:25 Sergey Senozhatsky
2017-10-03 7:20 ` Petr Mladek
2017-10-13 19:48 ` Geert Uytterhoeven
2017-10-13 20:44 ` Rob Herring
2017-10-12 17:24 ` Rob Herring
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).