Re: [PATCH] Sanitize possible_parent_show to Handle Return Value of of_clk_get_parent_name

[Stephen Boyd]

Quoting Alessandro Carminati (2023-09-08 01:36:14)
> Hello Stephen,
> Thank you for your review and the time you dedicated to it.
> 
> Il giorno ven 8 set 2023 alle ore 00:33 Stephen Boyd
> <sboyd@xxxxxxxxxx> ha scritto:
> >
> > Quoting Alessandro Carminati (2023-09-07 07:15:36)
> > > this is a gentle ping
> > >
> >
> > I couldn't read your email because it was sent to nobody
> > (unlisted-recipients). Can you resend with a proper To: line?
> >
> > >
> > > Il giorno mar 22 ago 2023 alle ore 15:49 Alessandro Carminati
> > > <alessandro.carminati@xxxxxxxxx> ha scritto:
> > > >
> > > > In the possible_parent_show function, ensure proper handling of the return
> > > > value from of_clk_get_parent_name to prevent potential issues arising from
> > > > a NULL return.
> > > > The current implementation invokes seq_puts directly on the result of
> > > > of_clk_get_parent_name without verifying the return value, which can lead
> > > > to kernel panic if the function returns NULL.
> > > >
> > > > This patch addresses the concern by introducing a check on the return
> > > > value of of_clk_get_parent_name. If the return value is not NULL, the
> >
> > Use of_clk_get_parent_name() to signify that it is a function.
> >
> > > > function proceeds to call seq_puts, providing the returned value as
> > > > argument.
> > > > However, if of_clk_get_parent_name returns NULL, the function provides a
> > > > static string as argument, avoiding the panic.
> > > >
> > > > Reported-by: Philip Daly <pdaly@xxxxxxxxxx>
> > > > Signed-off-by: Alessandro Carminati (Red Hat) <alessandro.carminati@xxxxxxxxx>
> > > > ---
> >
> > It needs a Fixes tag.
> Sure!
> I need to be more careful on this.
> 
> >
> > > >  drivers/clk/clk.c | 11 ++++++-----
> > > >  1 file changed, 6 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > > index c249f9791ae8..ab999644e185 100644
> > > > --- a/drivers/clk/clk.c
> > > > +++ b/drivers/clk/clk.c
> > > > @@ -3416,6 +3416,7 @@ static void possible_parent_show(struct seq_file *s, struct clk_core *core,
> > > >                                  unsigned int i, char terminator)
> > > >  {
> > > >         struct clk_core *parent;
> > > > +       const char *tmp;
> > > >
> > > >         /*
> > > >          * Go through the following options to fetch a parent's name.
> > > > @@ -3436,12 +3437,12 @@ static void possible_parent_show(struct seq_file *s, struct clk_core *core,
> > > >                 seq_puts(s, core->parents[i].name);
> > > >         else if (core->parents[i].fw_name)
> > > >                 seq_printf(s, "<%s>(fw)", core->parents[i].fw_name);
> > > > -       else if (core->parents[i].index >= 0)
> > > > -               seq_puts(s,
> > > > -                        of_clk_get_parent_name(core->of_node,
> > > > -                                               core->parents[i].index));
> > > > -       else
> > > > +       else if (core->parents[i].index >= 0) {
> > > > +               tmp = of_clk_get_parent_name(core->of_node, core->parents[i].index);
> > > > +               seq_puts(s, tmp ? tmp : "(none)");
> >
> > How about using seq_printf("%s", ...) instead? That should print out
> > "(null)" in the case that it is NULL, instead of "(none)" and it is a
> > one line change.
> 
> I did consider using seq_printf("%s", ...) as an alternative approach to
> address the issue initially.
> However, after a review of the file's history, I arrived at a different
> conclusion.
> 
> The commit [1] that introduced this bug was primarily focused on replacing
> seq_printf() with seq_putc().
> I considered that to maintain code consistency and align with the intentions
> of that commit, it may be more appropriate to continue using seq_putc() in
> this particular instance.
> I agree however with the idea of rolling back that particular change, but
> in this case, we perhaps may want to consider also [2], a similar change made
> in the same period.
> I haven't proceeded with a patch submission for it[2], mainly due to the lack
> of evidence of a kernel splash related to it and my uncertainty around the
> fact that can exist use cases where the name field in the struct cgroup_subsys
> can hit that code set to NULL.

Is nothing actually wrong? And this is a speculative patch?

All other arms of this conditional statement check the validity of the
pointer before printing the string. And when the parent isn't known we
print "(missing)", so it looks like we should do that instead. How about
this patch?

----8<---
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index c249f9791ae8..473563bc7496 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3416,6 +3416,7 @@ static void possible_parent_show(struct seq_file *s, struct clk_core *core,
 				 unsigned int i, char terminator)
 {
 	struct clk_core *parent;
+	const char *name = NULL;
 
 	/*
 	 * Go through the following options to fetch a parent's name.
@@ -3430,18 +3431,20 @@ static void possible_parent_show(struct seq_file *s, struct clk_core *core,
 	 * registered (yet).
 	 */
 	parent = clk_core_get_parent_by_index(core, i);
-	if (parent)
+	if (parent) {
 		seq_puts(s, parent->name);
-	else if (core->parents[i].name)
+	} else if (core->parents[i].name) {
 		seq_puts(s, core->parents[i].name);
-	else if (core->parents[i].fw_name)
+	} else if (core->parents[i].fw_name) {
 		seq_printf(s, "<%s>(fw)", core->parents[i].fw_name);
-	else if (core->parents[i].index >= 0)
-		seq_puts(s,
-			 of_clk_get_parent_name(core->of_node,
-						core->parents[i].index));
-	else
-		seq_puts(s, "(missing)");
+	} else {
+		if (core->parents[i].index >= 0)
+			name = of_clk_get_parent_name(core->of_node, core->parents[i].index);
+		if (!name)
+			name = "(missing)";
+
+		seq_puts(s, name);
+	}
 
 	seq_putc(s, terminator);
 }