Re: [PATCH] clk: imx: Fix a NULL pointer dereference in imx_register_uart_clocks()

[Abel Vesa]

On 22-01-28 16:50:54, Greg KH wrote:
> On Fri, Jan 28, 2022 at 07:47:06AM -0600, Adam Ford wrote:
> > On Fri, Jan 28, 2022 at 4:16 AM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > >
> > > On Tue, Jan 25, 2022 at 12:52:06AM +0800, Zhou Qingyang wrote:
> > > > In imx_register_uart_clocks(), the global variable imx_uart_clocks is
> > > > assigned by kcalloc() and there is a dereference of in the next for loop,
> > > > which could introduce a NULL pointer dereference on failure of kcalloc().
> > > >
> > > > Fix this by adding a NULL check of imx_uart_clocks.
> > > >
> > > > This bug was found by a static analyzer.
> > > >
> > > > Builds with 'make allyesconfig' show no new warnings,
> > > > and our static analyzer no longer warns about this code.
> > > >
> > > > Fixes: 379c9a24cc23 ("clk: imx: Fix reparenting of UARTs not associated with stdout")
> > > > Signed-off-by: Zhou Qingyang <zhou1615@xxxxxxx>
> > > > ---
> > > > The analysis employs differential checking to identify inconsistent
> > > > security operations (e.g., checks or kfrees) between two code paths
> > > > and confirms that the inconsistent operations are not recovered in the
> > > > current function or the callers, so they constitute bugs.
> > > >
> > > > Note that, as a bug found by static analysis, it can be a false
> > > > positive or hard to trigger. Multiple researchers have cross-reviewed
> > > > the bug.
> > > >
> > > >  drivers/clk/imx/clk.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c
> > > > index 7cc669934253..99249ab361d2 100644
> > > > --- a/drivers/clk/imx/clk.c
> > > > +++ b/drivers/clk/imx/clk.c
> > > > @@ -173,6 +173,8 @@ void imx_register_uart_clocks(unsigned int clk_count)
> > > >               int i;
> > > >
> > > >               imx_uart_clocks = kcalloc(clk_count, sizeof(struct clk *), GFP_KERNEL);
> > > > +             if (!imx_uart_clocks)
> > > > +                     return;
> > > >
> > > >               if (!of_stdout)
> > > >                       return;
> > > > --
> > > > 2.25.1
> > > >
> > >
> > > As stated before, umn.edu is still not allowed to contribute to the
> > > Linux kernel.  Please work with your administration to resolve this
> > > issue.

I'll ignore any patch from umn.edu until further notice.

> > 
> > Greg,
> > 
> > In the interest of safety, I believe this patch is reasonable.
> 
> How can kcalloc really fail here?  Seriously, this is an impossible
> thing to happen in real-world situations, you have to have special
> fault-injection tooling to ever hit this in a system that is not just
> frozen due to other problems.

Totally agree.

> 
> > I
> > wrote the original patch that is being fixed by this.  Would it be
> > acceptable if I submitted the same patch with "suggested-by"
> > associated with Zhou @ umn.edu?  I want to give credit where credit is
> > due while still maintaining the rule that patches are not actually
> > being accepted by umn.edu.
> 
> If you think this really is needed, then yes, feel free to rewrite it.
> 
> But rewrite it to be correct.  As it is, this is not correct.  If an
> error happens because we are out of memory, actually handle that and do
> not just return as if everything worked properly like this patch is
> doing here.
> 
> The "suggestion" here is incorrect, which is the big problem here.
> Whatever tool this group is using is wrong, and as a few people have
> hinted to me offline, maybe they are just still messing around with us
> and seeing how we behave.  Personally, I'm starting to get mad.

Lets consider this matter closed. The fix is quite pointless, TBH.

> 
> thanks,
> 
> greg k-h