linux-kernel - Re: [PATCH v1 2/3] cpufreq: mediatek: fix KP caused by handler usage after regulator_put/clk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <67024b47-f3c9-4764-9883-d52602cfefca@kili.mountain>
Date:   Fri, 10 Mar 2023 12:11:41 +0300
From:   Dan Carpenter <error27@...il.com>
To:     "jia-wei.chang" <jia-wei.chang@...iatek.com>
Cc:     "Rafael J . Wysocki" <rafael@...nel.org>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        Kevin Hilman <khilman@...libre.com>,
        Rex-BC Chen <rex-bc.chen@...iatek.com>,
        linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Project_Global_Chrome_Upstream_Group@...iatek.com,
        hsinyi@...gle.com, Nick <vincent@...temli.org>
Subject: Re: [PATCH v1 2/3] cpufreq: mediatek: fix KP caused by handler usage
 after regulator_put/clk_put

On Fri, Mar 10, 2023 at 01:17:49PM +0800, jia-wei.chang wrote:
> From: "Jia-Wei Chang" <jia-wei.chang@...iatek.com>
> 
> 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