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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20f261b3-5917-4e3e-96be-7dbb993c5a80@stanley.mountain>
Date: Wed, 9 Oct 2024 17:49:37 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Anand Moon <linux.amoon@...il.com>
Cc: oe-kbuild@...ts.linux.dev, Vinod Koul <vkoul@...nel.org>,
	Kishon Vijay Abraham I <kishon@...nel.org>,
	Heiko Stuebner <heiko@...ech.de>, linux-phy@...ts.infradead.org,
	linux-kernel@...r.kernel.org, lkp@...el.com,
	oe-kbuild-all@...ts.linux.dev
Subject: Re: [PATCH v2 2/3] phy: rockchip-pcie: Use devm_clk_get_enabled()
 helper

On Wed, Oct 09, 2024 at 07:59:38PM +0530, Anand Moon wrote:
> Hi Dan,
> 
> Thanks for the report.
> 
> On Wed, 9 Oct 2024 at 17:55, Dan Carpenter <dan.carpenter@...aro.org> wrote:
> >
> > Hi Anand,
> >
> > kernel test robot noticed the following build warnings:
> >
> > url:    https://github.com/intel-lab-lkp/linux/commits/Anand-Moon/phy-rockchip-pcie-Simplify-error-handling-with-dev_err_probe/20241007-115910
> > base:   8f602276d3902642fdc3429b548d73c745446601
> > patch link:    https://lore.kernel.org/r/20241007035616.2701-3-linux.amoon%40gmail.com
> > patch subject: [PATCH v2 2/3] phy: rockchip-pcie: Use devm_clk_get_enabled() helper
> > config: loongarch-randconfig-r071-20241009 (https://download.01.org/0day-ci/archive/20241009/202410092019.vGogfPIO-lkp@intel.com/config)
> > compiler: loongarch64-linux-gcc (GCC) 14.1.0
> >
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@...el.com>
> > | Reported-by: Dan Carpenter <dan.carpenter@...aro.org>
> > | Closes: https://lore.kernel.org/r/202410092019.vGogfPIO-lkp@intel.com/
> >
> > smatch warnings:
> > drivers/phy/rockchip/phy-rockchip-pcie.c:278 rockchip_pcie_phy_init() warn: missing error code 'err'
> >
> 
> All the functions in this file explicitly return 0 instead of err, I
> will fix this.
> 
> > vim +/err +278 drivers/phy/rockchip/phy-rockchip-pcie.c
> >
> > fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  269  static int rockchip_pcie_phy_init(struct phy *phy)
> > fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  270  {
> > 90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  271      struct phy_pcie_instance *inst = phy_get_drvdata(phy);
> > 90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  272      struct rockchip_pcie_phy *rk_phy = to_pcie_phy(inst);
> > fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  273      int err = 0;
> > fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  274
> > 90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  275      mutex_lock(&rk_phy->pcie_mutex);
> > 90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  276
> > 90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  277      if (rk_phy->init_cnt++)
> > 90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19 @278              goto err_out;
> >
> > Originally, this path just unlocked at returned zero.
> >
> > 90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  279
> > fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  280      err = reset_control_assert(rk_phy->phy_rst);
> > fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  281      if (err) {
> > fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  282              dev_err(&phy->dev, "assert phy_rst err %d\n", err);
> > 3114329651e74f drivers/phy/rockchip/phy-rockchip-pcie.c Anand Moon 2024-10-07  283              goto err_out;
> > fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  284      }
> > fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  285
> > 90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  286      mutex_unlock(&rk_phy->pcie_mutex);
> > 90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  287      return 0;
> > fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  288
> > 3114329651e74f drivers/phy/rockchip/phy-rockchip-pcie.c Anand Moon 2024-10-07  289  err_out:
> > 90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  290      rk_phy->init_cnt--;
> >
> > Now it decrements the counter so presumably it leads to an underflow/use after
> > free.
> 
> I was planning to replace the mutex_lock / mutex_unlock
> with guard(mutex)(&rk_phy->pcie_mutex) in the follow up patch.
> I will add this in the next revision.
> 
> >
> > 90a7612d070d5c drivers/phy/rockchip/phy-rockchip-pcie.c Shawn Lin  2017-07-19  291      mutex_unlock(&rk_phy->pcie_mutex);
> > fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  292      return err;
> > fcffee3d54fcad drivers/phy/phy-rockchip-pcie.c          Shawn Lin  2016-09-01  293  }
> >
> 

Thanks!

> Here are my modified changes on top of my changes for the review process.
> -----8<----------8<----------8<----------8<----------8<----------8<-----
> diff --git a/drivers/phy/rockchip/phy-rockchip-pcie.c
> b/drivers/phy/rockchip/phy-rockchip-pcie.c
> index 2c4d6f68f02a..09685dc3fe17 100644
> --- a/drivers/phy/rockchip/phy-rockchip-pcie.c
> +++ b/drivers/phy/rockchip/phy-rockchip-pcie.c
> @@ -248,20 +248,19 @@ static int rockchip_pcie_phy_init(struct phy *phy)
> 
>         mutex_lock(&rk_phy->pcie_mutex);
> 
> -       if (rk_phy->init_cnt++)
> -               goto err_out;
> +       if (rk_phy->init_cnt++) {
> +               mutex_unlock(&rk_phy->pcie_mutex);
> +               return err;

Please make this a return 0.  It's faster to not have to look up what a variable
is.

> +       }
> 
>         err = reset_control_assert(rk_phy->phy_rst);
>         if (err) {
>                 dev_err(&phy->dev, "assert phy_rst err %d\n", err);
> -               goto err_out;
> +               rk_phy->init_cnt--;
> +               mutex_unlock(&rk_phy->pcie_mutex);
> +               return err;
>         }
> 
> -       mutex_unlock(&rk_phy->pcie_mutex);
> -       return 0;
> -
> -err_out:
> -       rk_phy->init_cnt--;
>         mutex_unlock(&rk_phy->pcie_mutex);
>         return err;

return 0; here too.

regards,
dan carpenter


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ