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

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

[Zhou Qingyang]

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@umn.edu>
---
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

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

[Greg KH]

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@umn.edu>
> ---
> 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.

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

[Adam Ford]

On Fri, Jan 28, 2022 at 4:16 AM Greg KH <gregkh@linuxfoundation.org> 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@umn.edu>
> > ---
> > 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.

Greg,

In the interest of safety, I believe this patch is reasonable.  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.

adam

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

[Greg KH]

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@linuxfoundation.org> 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@umn.edu>
> > > ---
> > > 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.
> 
> 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.

> 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.

thanks,

greg k-h

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@linuxfoundation.org> 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@umn.edu>
> > > > ---
> > > > 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