[PATCH 0/3] clk: imx: imx8qxp-lpcg: Fix the clock-indices parsing

[PATCH 0/3] clk: imx: imx8qxp-lpcg: Fix the clock-indices parsing

[Abel Vesa]

imx_lpcg_parse_clks_from_dt() trusts the clock-indices property of the
LPCG node. Duplicate entries leak a clock and make the error path
unregister another one twice, and more entries than an LPCG has clock
outputs overflow the on-stack arrays the property and the related
clock names are read into. The first two patches reject both. The last
one makes a failure to read the parent names return an error code
instead of the number of clocks.

Prepared with help from an LLM-based assistant.

Signed-off-by: Abel Vesa <abel.vesa@oss.qualcomm.com>
---
Abel Vesa (3):
      clk: imx: imx8qxp-lpcg: Reject duplicate clock indices
      clk: imx: imx8qxp-lpcg: Fix stack overflow with too many clock indices
      clk: imx: imx8qxp-lpcg: Return an error if the parent names cannot be read

 drivers/clk/imx/clk-imx8qxp-lpcg.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)
---
base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
change-id: 20261102-imx8qxp-lpcg-duplicate-clock-indices-828fbaee36b0

Best regards,
--  

Abel

[PATCH 1/3] clk: imx: imx8qxp-lpcg: Reject duplicate clock indices

[Abel Vesa]

imx_lpcg_parse_clks_from_dt() stores each clock it registers in
clk_hws[bit_offset / 4], as every clock output of an LPCG owns a 4-bit
field. Nothing checks that the clock-indices property names each field
only once, though. If two entries fall into the same field, the second
registration overwrites the first one in clk_hws[], leaking it, and if
a later step fails, the error path computes the same index for both
entries and unregisters the second clock twice.

Reject a bit offset whose field already has a clock registered.

Fixes: d5f1e6a2bb61 ("clk: imx: imx8qxp-lpcg: add parsing clocks from device tree")
Cc: stable@vger.kernel.org
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/bug/linux-07b8d98e-feb9-4c88-afb6-3b5e0b05dc00
Signed-off-by: Abel Vesa <abel.vesa@oss.qualcomm.com>
---
 drivers/clk/imx/clk-imx8qxp-lpcg.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/clk/imx/clk-imx8qxp-lpcg.c b/drivers/clk/imx/clk-imx8qxp-lpcg.c
index f0cf6cf91899..e57e7bc074a9 100644
--- a/drivers/clk/imx/clk-imx8qxp-lpcg.c
+++ b/drivers/clk/imx/clk-imx8qxp-lpcg.c
@@ -251,6 +251,19 @@ static int imx_lpcg_parse_clks_from_dt(struct platform_device *pdev,
 			goto unreg;
 		}
 
+		/*
+		 * Each clock output owns a 4-bit field, so two bit offsets in
+		 * the same field would describe the same clock. Registering it
+		 * twice would leak the first registration, and the error path
+		 * would then unregister the second one twice.
+		 */
+		if (clk_hws[idx]) {
+			dev_warn(&pdev->dev, "duplicate bit offset of clock %d\n",
+				 i);
+			ret = -EINVAL;
+			goto unreg;
+		}
+
 		clk_hws[idx] = imx_clk_lpcg_scu_dev(&pdev->dev, output_names[i],
 						    parent_names[i], 0, base,
 						    bit_offset[i], false);

-- 
2.54.0

Re: [PATCH 1/3] clk: imx: imx8qxp-lpcg: Reject duplicate clock indices

[Frank Li]

On Mon, Nov 02, 2026 at 01:48:49PM +0200, Abel Vesa wrote:
> imx_lpcg_parse_clks_from_dt() stores each clock it registers in
> clk_hws[bit_offset / 4], as every clock output of an LPCG owns a 4-bit
> field. Nothing checks that the clock-indices property names each field
> only once, though. If two entries fall into the same field, the second
> registration overwrites the first one in clk_hws[], leaking it, and if
> a later step fails, the error path computes the same index for both
> entries and unregisters the second clock twice.
>
> Reject a bit offset whose field already has a clock registered.
>
> Fixes: d5f1e6a2bb61 ("clk: imx: imx8qxp-lpcg: add parsing clocks from device tree")
> Cc: stable@vger.kernel.org
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://sashiko.dev/#/bug/linux-07b8d98e-feb9-4c88-afb6-3b5e0b05dc00
> Signed-off-by: Abel Vesa <abel.vesa@oss.qualcomm.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/clk/imx/clk-imx8qxp-lpcg.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/drivers/clk/imx/clk-imx8qxp-lpcg.c b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> index f0cf6cf91899..e57e7bc074a9 100644
> --- a/drivers/clk/imx/clk-imx8qxp-lpcg.c
> +++ b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> @@ -251,6 +251,19 @@ static int imx_lpcg_parse_clks_from_dt(struct platform_device *pdev,
>  			goto unreg;
>  		}
>
> +		/*
> +		 * Each clock output owns a 4-bit field, so two bit offsets in
> +		 * the same field would describe the same clock. Registering it
> +		 * twice would leak the first registration, and the error path
> +		 * would then unregister the second one twice.
> +		 */
> +		if (clk_hws[idx]) {
> +			dev_warn(&pdev->dev, "duplicate bit offset of clock %d\n",
> +				 i);
> +			ret = -EINVAL;
> +			goto unreg;
> +		}
> +
>  		clk_hws[idx] = imx_clk_lpcg_scu_dev(&pdev->dev, output_names[i],
>  						    parent_names[i], 0, base,
>  						    bit_offset[i], false);
>
> --
> 2.54.0
>
>

[PATCH 2/3] clk: imx: imx8qxp-lpcg: Fix stack overflow with too many clock indices

[Abel Vesa]

imx_lpcg_parse_clks_from_dt() reads the clock-indices, the parent names
and the clock-output-names into on-stack arrays of IMX_LPCG_MAX_CLKS
entries, but sizes the reads by the number of clock-indices entries,
which it never checks. A clock-indices property with more than
IMX_LPCG_MAX_CLKS entries thus makes all three reads write past the end
of their arrays.

An LPCG has at most IMX_LPCG_MAX_CLKS clock outputs, so reject a larger
count.

Fixes: d5f1e6a2bb61 ("clk: imx: imx8qxp-lpcg: add parsing clocks from device tree")
Cc: stable@vger.kernel.org
Signed-off-by: Abel Vesa <abel.vesa@oss.qualcomm.com>
---
 drivers/clk/imx/clk-imx8qxp-lpcg.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/clk/imx/clk-imx8qxp-lpcg.c b/drivers/clk/imx/clk-imx8qxp-lpcg.c
index e57e7bc074a9..ae8cb5408246 100644
--- a/drivers/clk/imx/clk-imx8qxp-lpcg.c
+++ b/drivers/clk/imx/clk-imx8qxp-lpcg.c
@@ -200,6 +200,11 @@ static int imx_lpcg_parse_clks_from_dt(struct platform_device *pdev,
 		return -EINVAL;
 	}
 
+	if (count > IMX_LPCG_MAX_CLKS) {
+		dev_err(&pdev->dev, "too many clocks: %d\n", count);
+		return -EINVAL;
+	}
+
 	/*
 	 * A trick here is that we set the num of clks to the MAX instead
 	 * of the count from clock-indices because one LPCG supports up to

-- 
2.54.0

Re: [PATCH 2/3] clk: imx: imx8qxp-lpcg: Fix stack overflow with too many clock indices

[Frank Li]

On Mon, Nov 02, 2026 at 01:48:50PM +0200, Abel Vesa wrote:
> imx_lpcg_parse_clks_from_dt() reads the clock-indices, the parent names
> and the clock-output-names into on-stack arrays of IMX_LPCG_MAX_CLKS
> entries, but sizes the reads by the number of clock-indices entries,
> which it never checks. A clock-indices property with more than
> IMX_LPCG_MAX_CLKS entries thus makes all three reads write past the end
> of their arrays.
>
> An LPCG has at most IMX_LPCG_MAX_CLKS clock outputs, so reject a larger
> count.
>
> Fixes: d5f1e6a2bb61 ("clk: imx: imx8qxp-lpcg: add parsing clocks from device tree")
> Cc: stable@vger.kernel.org
> Signed-off-by: Abel Vesa <abel.vesa@oss.qualcomm.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/clk/imx/clk-imx8qxp-lpcg.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/clk/imx/clk-imx8qxp-lpcg.c b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> index e57e7bc074a9..ae8cb5408246 100644
> --- a/drivers/clk/imx/clk-imx8qxp-lpcg.c
> +++ b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> @@ -200,6 +200,11 @@ static int imx_lpcg_parse_clks_from_dt(struct platform_device *pdev,
>  		return -EINVAL;
>  	}
>
> +	if (count > IMX_LPCG_MAX_CLKS) {
> +		dev_err(&pdev->dev, "too many clocks: %d\n", count);
> +		return -EINVAL;
> +	}
> +
>  	/*
>  	 * A trick here is that we set the num of clks to the MAX instead
>  	 * of the count from clock-indices because one LPCG supports up to
>
> --
> 2.54.0
>
>

[PATCH 3/3] clk: imx: imx8qxp-lpcg: Return an error if the parent names cannot be read

[Abel Vesa]

When of_clk_parent_fill() does not fill in a parent name for every
clock, imx_lpcg_parse_clks_from_dt() returns the number of clocks
instead of an error code. The probe then falls back to the legacy
binding as it does on any error, but a positive value is not what the
function is meant to return on failure.

Return -EINVAL instead, like the other parsing errors do.

Fixes: d5f1e6a2bb61 ("clk: imx: imx8qxp-lpcg: add parsing clocks from device tree")
Signed-off-by: Abel Vesa <abel.vesa@oss.qualcomm.com>
---
 drivers/clk/imx/clk-imx8qxp-lpcg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/imx/clk-imx8qxp-lpcg.c b/drivers/clk/imx/clk-imx8qxp-lpcg.c
index ae8cb5408246..4070e06f9c7d 100644
--- a/drivers/clk/imx/clk-imx8qxp-lpcg.c
+++ b/drivers/clk/imx/clk-imx8qxp-lpcg.c
@@ -231,7 +231,7 @@ static int imx_lpcg_parse_clks_from_dt(struct platform_device *pdev,
 	ret = of_clk_parent_fill(np, parent_names, count);
 	if (ret != count) {
 		dev_err(&pdev->dev, "failed to get clock parent names\n");
-		return count;
+		return -EINVAL;
 	}
 
 	ret = of_property_read_string_array(np, "clock-output-names",

-- 
2.54.0

Re: [PATCH 3/3] clk: imx: imx8qxp-lpcg: Return an error if the parent names cannot be read

[Frank Li]

On Mon, Nov 02, 2026 at 01:48:51PM +0200, Abel Vesa wrote:
> When of_clk_parent_fill() does not fill in a parent name for every
> clock, imx_lpcg_parse_clks_from_dt() returns the number of clocks
> instead of an error code. The probe then falls back to the legacy
> binding as it does on any error, but a positive value is not what the
> function is meant to return on failure.
>
> Return -EINVAL instead, like the other parsing errors do.
>
> Fixes: d5f1e6a2bb61 ("clk: imx: imx8qxp-lpcg: add parsing clocks from device tree")
> Signed-off-by: Abel Vesa <abel.vesa@oss.qualcomm.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/clk/imx/clk-imx8qxp-lpcg.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/imx/clk-imx8qxp-lpcg.c b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> index ae8cb5408246..4070e06f9c7d 100644
> --- a/drivers/clk/imx/clk-imx8qxp-lpcg.c
> +++ b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> @@ -231,7 +231,7 @@ static int imx_lpcg_parse_clks_from_dt(struct platform_device *pdev,
>  	ret = of_clk_parent_fill(np, parent_names, count);
>  	if (ret != count) {
>  		dev_err(&pdev->dev, "failed to get clock parent names\n");
> -		return count;
> +		return -EINVAL;
>  	}
>
>  	ret = of_property_read_string_array(np, "clock-output-names",
>
> --
> 2.54.0
>
>

Re: [PATCH 0/3] clk: imx: imx8qxp-lpcg: Fix the clock-indices parsing

[Peng Fan]

On Mon, Nov 02, 2026 at 01:48:48PM +0200, Abel Vesa wrote:
>imx_lpcg_parse_clks_from_dt() trusts the clock-indices property of the
>LPCG node. Duplicate entries leak a clock and make the error path
>unregister another one twice, and more entries than an LPCG has clock
>outputs overflow the on-stack arrays the property and the related
>clock names are read into. The first two patches reject both. The last
>one makes a failure to read the parent names return an error code
>instead of the number of clocks.
>
>Prepared with help from an LLM-based assistant.
>
>Signed-off-by: Abel Vesa <abel.vesa@oss.qualcomm.com>
>---
>Abel Vesa (3):
>      clk: imx: imx8qxp-lpcg: Reject duplicate clock indices
>      clk: imx: imx8qxp-lpcg: Fix stack overflow with too many clock indices
>      clk: imx: imx8qxp-lpcg: Return an error if the parent names cannot be read
>

Reviewed-by: Peng Fan <peng.fan@nxp.com>