[PATCH 0/2] phy: Make sure clk_init_data is fully initialized

[PATCH 0/2] phy: Make sure clk_init_data is fully initialized

[Geert Uytterhoeven]

	Hi all,

The clk_init_data structure contains several mutually-exclusive members
for different methods to specify the possible parents of a clock,
prompting drivers to initialize only the members they need.  However,
not initializing all members may cause subtle issues, which are only
exposed when CONFIG_INIT_STACK_ALL_PATTERN or CONFIG_INIT_STACK_NONE is
enabled.

Hence this series aims to make sure all members are fully initialized,
to avoid such bugs, and to prevent future breakage when converting
drivers to a different method for specifying the parents.

Part One[1] fixed all cases that I identified to be real bugs, in
response to a crash I saw on BeagleBone Black.

This series is the PHY subpart of Part Two, which fixes remaining cases
that are currently harmless.  These are still fragile, and may cause
future breakage when converting drivers to a different method for
specifying the parents.

Thanks for your comments!

[1] "[PATCH treewide 0/5] clk: Make sure clk_init_data is fully
     initialized (part 1)"
    https://lore.kernel.org/cover.1787165329.git.geert+renesas@glider.be

Geert Uytterhoeven (2):
  phy: freescale: fsl-samsung-hdmi: Make sure clk_init_data is fully
    initialized
  phy: rockchip: Make sure clk_init_data is fully initialized

 drivers/phy/freescale/phy-fsl-samsung-hdmi.c  | 2 +-
 drivers/phy/rockchip/phy-rockchip-inno-hdmi.c | 2 +-
 drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 2 +-
 drivers/phy/rockchip/phy-rockchip-usb.c       | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

-- 
2.43.0

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-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

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH 1/2] phy: freescale: fsl-samsung-hdmi: Make sure clk_init_data is fully initialized

[Geert Uytterhoeven]

The clk_init_data structure contains several mutually-exclusive members
for different methods to specify the possible parents of a clock,
prompting drivers to initialize only the members they need.  However,
not initializing all members may cause subtle issues, which are only
exposed when CONFIG_INIT_STACK_ALL_PATTERN or CONFIG_INIT_STACK_NONE is
enabled.

Make sure all members are fully initialized, to avoid such bugs, and to
prevent future breakage when converting drivers to a different method
for specifying the parents.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Compile-tested only.
---
 drivers/phy/freescale/phy-fsl-samsung-hdmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
index 0f25d81de61bb67f..ded142020e725f58 100644
--- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
+++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
@@ -610,7 +610,7 @@ static int phy_clk_register(struct fsl_samsung_hdmi_phy *phy)
 {
 	struct device *dev = phy->dev;
 	struct device_node *np = dev->of_node;
-	struct clk_init_data init;
+	struct clk_init_data init = {};
 	const char *parent_name;
 	struct clk *phyclk;
 	int ret;
-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH 1/2] phy: freescale: fsl-samsung-hdmi: Make sure clk_init_data is fully initialized

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] A global static variable is mutated during clock rate queries, causing data races and state corruption with PM resume operations.
--

--- Patch [1]: [PATCH 1/2] phy: freescale: fsl-samsung-hdmi: Make sure clk_init_data is fully initialized ---
commit eace776c1e0b617b09367e8f7566adff67211a9f
Author: Geert Uytterhoeven <geert+renesas@glider.be>

phy: freescale: fsl-samsung-hdmi: Make sure clk_init_data is fully initialized

The clk_init_data structure contains several mutually-exclusive members
for different methods to specify the possible parents of a clock,
prompting drivers to initialize only the members they need.  However,
not initializing all members may cause subtle issues, which are only
exposed when CONFIG_INIT_STACK_ALL_PATTERN or CONFIG_INIT_STACK_NONE is
enabled.

> diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> index d010fec15671d..76c0d571e3155 100644
> --- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> +++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> @@ -610,7 +610,7 @@ static int phy_clk_register(struct fsl_samsung_hdmi_phy *phy)
>  {
>  	struct device *dev = phy->dev;
>  	struct device_node *np = dev->of_node;
> -	struct clk_init_data init;
> +	struct clk_init_data init = {};
>  	const char *parent_name;
>  	struct clk *phyclk;
>  	int ret;

[Severity: High]
This is a pre-existing issue, but does mutating the global static variable
calculated_phy_pll_cfg during side-effect-free clock queries cause state
corruption?

In fsl_samsung_hdmi_phy_find_settings(), the global variable is modified
unconditionally:

drivers/phy/freescale/phy-fsl-samsung-hdmi.c:fsl_samsung_hdmi_phy_find_settings() {
    ...
	/* Calculate the integer divider */
	int_div_clk = fsl_samsung_hdmi_phy_find_pms(rate, &p, &m, &s);
	fsl_samsung_hdmi_calculate_phy(&calculated_phy_pll_cfg, int_div_clk, p, m, s);
    ...
}

Then in fsl_samsung_hdmi_phy_configure(), a pointer to this global is saved:

drivers/phy/freescale/phy-fsl-samsung-hdmi.c:fsl_samsung_hdmi_phy_configure() {
    ...
	phy->cur_cfg = cfg;
    ...
}

If a clock driver sets a rate (saving the pointer) and later performs a
clk_round_rate() query, the query will overwrite calculated_phy_pll_cfg.

When the device resumes, it restores the hardware using this potentially
corrupted pointer:

drivers/phy/freescale/phy-fsl-samsung-hdmi.c:fsl_samsung_hdmi_phy_resume() {
    ...
	if (phy->cur_cfg)
		ret = fsl_samsung_hdmi_phy_configure(phy, phy->cur_cfg);
    ...
}

Can this lead to the hardware being restored to an incorrect frequency
upon PM resume?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1787240165.git.geert+renesas@glider.be?part=1

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH 2/2] phy: rockchip: Make sure clk_init_data is fully initialized

[Geert Uytterhoeven]

The clk_init_data structure contains several mutually-exclusive members
for different methods to specify the possible parents of a clock,
prompting drivers to initialize only the members they need.  However,
not initializing all members may cause subtle issues, which are only
exposed when CONFIG_INIT_STACK_ALL_PATTERN or CONFIG_INIT_STACK_NONE is
enabled.

Make sure all members are fully initialized, to avoid such bugs, and to
prevent future breakage when converting drivers to a different method
for specifying the parents.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Compile-tested only.
---
 drivers/phy/rockchip/phy-rockchip-inno-hdmi.c | 2 +-
 drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 2 +-
 drivers/phy/rockchip/phy-rockchip-usb.c       | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c b/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c
index c3d257a0f4a65b4c..d2c2b17df5f340e4 100644
--- a/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c
+++ b/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c
@@ -1044,7 +1044,7 @@ static int inno_hdmi_phy_clk_register(struct inno_hdmi_phy *inno)
 {
 	struct device *dev = inno->dev;
 	struct device_node *np = dev->of_node;
-	struct clk_init_data init;
+	struct clk_init_data init = {};
 	const char *parent_name;
 	int ret;
 
diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
index 7d8a533f24aeadc2..e47b89015586feee 100644
--- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
+++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
@@ -400,7 +400,7 @@ static int
 rockchip_usb2phy_clk480m_register(struct rockchip_usb2phy *rphy)
 {
 	struct device_node *node = rphy->dev->of_node;
-	struct clk_init_data init;
+	struct clk_init_data init = {};
 	struct clk *refclk = NULL;
 	const char *clk_name;
 	int i;
diff --git a/drivers/phy/rockchip/phy-rockchip-usb.c b/drivers/phy/rockchip/phy-rockchip-usb.c
index cef96739cf3ff41f..c5a2853a63d3099c 100644
--- a/drivers/phy/rockchip/phy-rockchip-usb.c
+++ b/drivers/phy/rockchip/phy-rockchip-usb.c
@@ -200,9 +200,9 @@ static int rockchip_usb_phy_init(struct rockchip_usb_phy_base *base,
 				 struct device_node *child)
 {
 	struct rockchip_usb_phy *rk_phy;
+	struct clk_init_data init = {};
 	unsigned int reg_offset;
 	const char *clk_name;
-	struct clk_init_data init;
 	int err, i;
 
 	rk_phy = devm_kzalloc(base->dev, sizeof(*rk_phy), GFP_KERNEL);
-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH 2/2] phy: rockchip: Make sure clk_init_data is fully initialized

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Incorrect validation of the `refclk` pointer results in registering a clock with 1 parent but a NULL parent name if the optional 'phyclk' is missing.
- [High] Missing `of_clk_del_provider` on the error path of `inno_hdmi_phy_probe`, leading to a use-after-free in the OF clock provider list.
--

--- Patch [2]: [PATCH 2/2] phy: rockchip: Make sure clk_init_data is fully initialized ---
commit 830443ad1b9c48211b68c5bb691a7101b0e76d09
Author: Geert Uytterhoeven <geert+renesas@glider.be>

phy: rockchip: Make sure clk_init_data is fully initialized

This commit zero-initializes the clk_init_data structures across several
rockchip phy drivers to ensure all mutually-exclusive members are cleared.
This prevents subtle bugs when specific initialization configurations are
enabled.

> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c b/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c
> index 1483907413faa..80aeed965c3f3 100644
> --- a/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c
> @@ -999,7 +999,7 @@ static int inno_hdmi_phy_clk_register(struct inno_hdmi_phy *inno)
>  {
>  	struct device *dev = inno->dev;
>  	struct device_node *np = dev->of_node;
> -	struct clk_init_data init;
> +	struct clk_init_data init = {};
>  	const char *parent_name;
>  	int ret;

[Severity: High]
This isn't a bug introduced by this patch, but does the clock provider
registration here leak on the probe error path?

In inno_hdmi_phy_probe(), if devm_of_phy_provider_register() fails after
the clock is registered:

inno_hdmi_phy_probe() {
    ...
    ret = inno_hdmi_phy_clk_register(inno);
        // Calls of_clk_add_provider() which adds to global list
    ...
    phy_provider = devm_of_phy_provider_register(...);
    return PTR_ERR_OR_ZERO(phy_provider);
}

If the phy provider registration fails, the probe aborts without calling
of_clk_del_provider(). Devres will free inno->phyclk, leaving a dangling
pointer in the global of_clk_providers list which could trigger a
use-after-free during subsequent clock lookups.

> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> index 7d8a533f24aea..e47b89015586f 100644
> --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> @@ -400,7 +400,7 @@ static int
>  rockchip_usb2phy_clk480m_register(struct rockchip_usb2phy *rphy)
>  {
>  	struct device_node *node = rphy->dev->of_node;
> -	struct clk_init_data init;
> +	struct clk_init_data init = {};
>  	struct clk *refclk = NULL;
>  	const char *clk_name;
>  	int i;

[Severity: High]
This is a pre-existing issue, but does this function correctly handle
missing optional clocks later in the logic?

If the optional phyclk is not found, refclk remains NULL. However, the
validation check further down evaluates NULL as true:

rockchip_usb2phy_clk480m_register() {
    ...
    if (!IS_ERR(refclk)) {
        clk_name = __clk_get_name(refclk);
        init.parent_names = &clk_name;
        init.num_parents = 1;
    } else {
        ...
    }
}

Since !IS_ERR(NULL) is true, it attempts to register the clock with 1 parent
and a NULL parent name, causing clk_register() to fail with -EINVAL and
breaking USB PHY support on platforms missing this clock.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1787240165.git.geert+renesas@glider.be?part=2

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy