Re: [PATCH v1 2/3] cpufreq: mediatek: fix KP caused by handler usage after regulator_put/clk_put

[Dan Carpenter]

On Fri, Mar 10, 2023 at 01:17:49PM +0800, jia-wei.chang wrote:
> From: "Jia-Wei Chang" <jia-wei.chang@xxxxxxxxxxxx>
> 
> Any kind of failure in mtk_cpu_dvfs_info_init() will lead to calling
> regulator_put() or clk_put() and the KP will occur since the regulator/clk
> handlers are used after released in mtk_cpu_dvfs_info_release().
> 

This patch is harmless but it's not required.

If the mtk_cpu_dvfs_info_init() function is not able to complete
successfully then it cleans up all the partial allocations.  If it is
able to allocate everything successfully, then the caller, which is
mtk_cpufreq_probe(), adds it to the &dvfs_info_list.  If the
probe() function is not able to complete successfully it releases
everything in the &dvfs_info_list.

In this way, mtk_cpu_dvfs_info_init() never frees anything which has
not been allocated and it never frees anything which has already been
freed.  All the IS_ERR() checks in mtk_cpu_dvfs_info_release() can be
removed.

In reviewing this code, I can't help but notice that mtk_cpu_dvfs_info_init()
is buggy.  Please read my blog on error handling:
https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/

drivers/cpufreq/mediatek-cpufreq.c
   411          info->cpu_clk = clk_get(cpu_dev, "cpu");
   412          if (IS_ERR(info->cpu_clk)) {
   413                  ret = PTR_ERR(info->cpu_clk);
   414                  return dev_err_probe(cpu_dev, ret,
                        ^^^^^^^^^^^^^^^^^^^^
Here we have a direct return which means there is nothing to free.
Okay.

   415                                       "cpu%d: failed to get cpu clk\n", cpu);
   416          }
   417  
   418          info->inter_clk = clk_get(cpu_dev, "intermediate");
   419          if (IS_ERR(info->inter_clk)) {
   420                  ret = PTR_ERR(info->inter_clk);
   421                  dev_err_probe(cpu_dev, ret,
   422                                "cpu%d: failed to get intermediate clk\n", cpu);
   423                  goto out_free_resources;
                        ^^^^^^^^^^^^^^^^^^^^^^^
The last thing we successfully allocated was ->cpu_clk so ideally the
goto would be something like "goto put_cpu_clk;".  Let's assume this
clk_get() fails.


   424          }
   425  
   426          info->proc_reg = regulator_get_optional(cpu_dev, "proc");
                ^^^^^^^^^^^^^^
Note that this is where ->proc_reg is allocated.  Prior to that point
it is NULL because it's allocated with kzalloc().

   427          if (IS_ERR(info->proc_reg)) {
   428                  ret = PTR_ERR(info->proc_reg);
   429                  dev_err_probe(cpu_dev, ret,
   430                                "cpu%d: failed to get proc regulator\n", cpu);
   431                  goto out_free_resources;
   432          }

[ snip ]

   536  out_free_resources:
   537          if (regulator_is_enabled(info->proc_reg))
                                         ^^^^^^^^^^^^^^
Oops!  This is a NULL dereference because ->proc_reg wasn't allocated
yet.

   538                  regulator_disable(info->proc_reg);
   539          if (info->sram_reg && regulator_is_enabled(info->sram_reg))
   540                  regulator_disable(info->sram_reg);
   541  
   542          if (!IS_ERR(info->proc_reg))
   543                  regulator_put(info->proc_reg);
   544          if (!IS_ERR(info->sram_reg))
   545                  regulator_put(info->sram_reg);
   546          if (!IS_ERR(info->cpu_clk))
   547                  clk_put(info->cpu_clk);
   548          if (!IS_ERR(info->inter_clk))
   549                  clk_put(info->inter_clk);
   550  
   551          return ret;
   552  }

It would be safer and more readable to have:

err_put_inter_clk:
	clk_put(info->inter_clk);
err_put_cpu_clk:
	clk_put(info->cpu_clk);

etc.

regards,
dan carpenter