linux-kernel - RE: [PATCH v2] scsi: ufs: core: Ensure clk_gating.lock is used only after initialization

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:
 <DM6PR04MB657531813CCA81C40A7D65E5FCE32@DM6PR04MB6575.namprd04.prod.outlook.com>
Date: Fri, 24 Jan 2025 12:48:47 +0000
From: Avri Altman <Avri.Altman@....com>
To: Geert Uytterhoeven <geert@...ux-m68k.org>
CC: "Martin K . Petersen" <martin.petersen@...cle.com>,
	"linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Manivannan
 Sadhasivam <manisadhasivam.linux@...il.com>, Bart Van Assche
	<bvanassche@....org>, Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Subject: RE: [PATCH v2] scsi: ufs: core: Ensure clk_gating.lock is used only
 after initialization

> > +       /*
> > +        * Initialize clk_gating.lock early since it is being used in
> > +        * ufshcd_setup_clocks()
> > +        */
> > +       if (ufshcd_is_clkgating_allowed(hba))
> 
> This checks if the UFSHCD_CAP_CLK_GATING flag is set, which is only done in a
> subset of the host drivers:
> 
> drivers/ufs/host/ufs-exynos.c:  hba->caps |= UFSHCD_CAP_CLK_GATING |
> UFSHCD_CAP_HIBERN8_WITH_CLK_GATING;
> drivers/ufs/host/ufs-mediatek.c:        hba->caps |= UFSHCD_CAP_CLK_GATING;
> drivers/ufs/host/ufs-qcom.c:    hba->caps |= UFSHCD_CAP_CLK_GATING |
> UFSHCD_CAP_HIBERN8_WITH_CLK_GATING;
> drivers/ufs/host/ufs-sprd.c:    hba->caps |= UFSHCD_CAP_CLK_GATING |
> 
> > +               spin_lock_init(&hba->clk_gating.lock);
> 
> Hence the spinlock is not uninitialized on all other host drivers.
Oh - Thanks for pointing this out.  I totally missed it.
At is appears when clock_gating was invented - 1ab27c9cf8b6 ("ufs: Add support for clock gating")
The below was added without checking if ufshcd_is_clkgating_allowed, or better yet - clk_gating.is_initialized:

@@ -4151,12 +4420,19 @@ static int __ufshcd_setup_clocks(struct ufs_hba *hba, bool on,
                                        clki->name, on ? "en" : "dis");
                }
        }
+       } else if (!ret && on) {
+               spin_lock_irqsave(hba->host->host_lock, flags);
+               hba->clk_gating.state = CLKS_ON;
+               spin_unlock_irqrestore(hba->host->host_lock, flags);
        }

Looks like this is the missing piece.
Will fix this in a 2nd patch.

Thanks,
Avri

> 
> > +
> >         err = ufshcd_hba_init(hba);
> >         if (err)
> >                 goto out_error;
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-
> m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But when
> I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds