[PATCH 5.15.y] phy: qcom: qmp: Fix NULL pointer dereference for USB Uni PHYs

[PATCH 5.15.y] phy: qcom: qmp: Fix NULL pointer dereference for USB Uni PHYs

[Krishna Kurapati]

Commit [1] introduced DP support to QMP driver. While doing so, the
dp and usb configuration structures were added to a combo_phy_cfg
structure. During probe, the match data is used to parse and identify the
dp and usb configs separately. While doing so, the usb_cfg variable
represents the configuration parameters for USB part of the phy (whether
it is DP-Cobo or Uni). during probe, one corner case of parsing usb_cfg
for Uni PHYs is left incomplete and it is left as NULL. This NULL variable
further percolates down to qmp_phy_create() call essentially getting
de-referenced and causing a crash.

Subsequently, commit [2] split the driver into multiple files, each
handling a specific PHY type (USB, DP-Combo, UFS, PCIe). During this
refactoring, the probing process was modified, and the NULL pointer
dereference issue no longer showed up.

[1]: https://lore.kernel.org/all/20200916231202.3637932-8-swboyd@chromium.org/
[2]: https://lore.kernel.org/all/20220607213203.2819885-1-dmitry.baryshkov@linaro.org/

Fixes: 52e013d0bffa ("phy: qcom-qmp: Add support for DP in USB3+DP combo phy")
Cc: stable@vger.kernel.org # 5.15.y
Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
---
 drivers/phy/qualcomm/phy-qcom-qmp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
index eef863108bfe..e22ee71aa060 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -5714,6 +5714,8 @@ static int qcom_qmp_phy_probe(struct platform_device *pdev)
 
 		usb_cfg = combo_cfg->usb_cfg;
 		cfg = usb_cfg; /* Setup clks and regulators */
+	} else {
+		usb_cfg = cfg;
 	}
 
 	/* per PHY serdes; usually located at base address */
-- 
2.34.1

Re: [PATCH 5.15.y] phy: qcom: qmp: Fix NULL pointer dereference for USB Uni PHYs

[Dmitry Baryshkov]

On Fri, Nov 15, 2024 at 02:45:45PM +0530, Krishna Kurapati wrote:
> Commit [1] introduced DP support to QMP driver. While doing so, the
> dp and usb configuration structures were added to a combo_phy_cfg
> structure. During probe, the match data is used to parse and identify the
> dp and usb configs separately. While doing so, the usb_cfg variable
> represents the configuration parameters for USB part of the phy (whether
> it is DP-Cobo or Uni). during probe, one corner case of parsing usb_cfg
> for Uni PHYs is left incomplete and it is left as NULL. This NULL variable
> further percolates down to qmp_phy_create() call essentially getting
> de-referenced and causing a crash.

The UNI PHY platforms don't have usb3-phy subnode. As such the usb_cfg
variable should not be used in the for_each_available_child_of_node()
loop.

Please provide details for the platform on which you observe the crash
and the backtrace.

> 
> Subsequently, commit [2] split the driver into multiple files, each
> handling a specific PHY type (USB, DP-Combo, UFS, PCIe). During this
> refactoring, the probing process was modified, and the NULL pointer
> dereference issue no longer showed up.
> 
> [1]: https://lore.kernel.org/all/20200916231202.3637932-8-swboyd@chromium.org/
> [2]: https://lore.kernel.org/all/20220607213203.2819885-1-dmitry.baryshkov@linaro.org/
> 
> Fixes: 52e013d0bffa ("phy: qcom-qmp: Add support for DP in USB3+DP combo phy")
> Cc: stable@vger.kernel.org # 5.15.y
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> index eef863108bfe..e22ee71aa060 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> @@ -5714,6 +5714,8 @@ static int qcom_qmp_phy_probe(struct platform_device *pdev)
>  
>  		usb_cfg = combo_cfg->usb_cfg;
>  		cfg = usb_cfg; /* Setup clks and regulators */
> +	} else {
> +		usb_cfg = cfg;
>  	}
>  
>  	/* per PHY serdes; usually located at base address */
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH 5.15.y] phy: qcom: qmp: Fix NULL pointer dereference for USB Uni PHYs

[Krishna Kurapati]

On 11/15/2024 9:29 PM, Dmitry Baryshkov wrote:
> On Fri, Nov 15, 2024 at 02:45:45PM +0530, Krishna Kurapati wrote:
>> Commit [1] introduced DP support to QMP driver. While doing so, the
>> dp and usb configuration structures were added to a combo_phy_cfg
>> structure. During probe, the match data is used to parse and identify the
>> dp and usb configs separately. While doing so, the usb_cfg variable
>> represents the configuration parameters for USB part of the phy (whether
>> it is DP-Cobo or Uni). during probe, one corner case of parsing usb_cfg
>> for Uni PHYs is left incomplete and it is left as NULL. This NULL variable
>> further percolates down to qmp_phy_create() call essentially getting
>> de-referenced and causing a crash.
> 
> The UNI PHY platforms don't have usb3-phy subnode. As such the usb_cfg
> variable should not be used in the for_each_available_child_of_node()
> loop.
> 
> Please provide details for the platform on which you observe the crash
> and the backtrace.
> 

I got this error when I started working on multiport support (begining 
of 2023). Initially I tried testing on my code on 5.15, hence this patch 
was raised on the same.

The 2 qmp phys on sa8195 and sa8295 (based on sc8280xp) are uni phy and 
the following was the DT node that worked out for me on 5.15 codebase:


	usb_1_qmpphy: ssphy@88eb000 {
		compatible = "qcom,sm8150-qmp-usb3-uni-phy";
		reg = <0x088eb000 0x200>;
		#address-cells = <1>;
		#size-cells = <1>;
		ranges;
		//status = "disabled";
		clocks = <&gcc GCC_USB3_MP_PHY_AUX_CLK>,
			 <&rpmhcc RPMH_CXO_CLK>,
			 <&gcc GCC_USB3_SEC_CLKREF_CLK>,
			 <&gcc GCC_USB3_MP_PHY_COM_AUX_CLK>;
		clock-names = "aux", "ref_clk_src", "ref", "com_aux";

		resets = <&gcc GCC_USB3UNIPHY_PHY_MP0_BCR>,
			 <&gcc GCC_USB3_UNIPHY_MP0_BCR>;
		reset-names = "phy", "common";

		//vdda-phy-supply = <&L3C>;
		vdda-pll-supply = <&L5E>;

		usb_1_ssphy: usb3-phy@88eb200 {
			reg = <0x088eb200 0x200>,
			      <0x088eb400 0x200>,
			      <0x088eb800 0x800>,
			      <0x088eb600 0x200>;
			#clock-cells = <0>;
			#phy-cells = <0>;
			clocks = <&gcc GCC_USB3_MP_PHY_PIPE_0_CLK>;
			clock-names = "pipe0";
			clock-output-names = "usb3_uni_phy_pipe_clk_src";
		};
	};


I was hitting the bug when I write the DT above way on top of 5.15 baseline.

In 5.15.y, the SM8150 usb_2_qmpphy dT is as follows:

                 usb_2_qmpphy: phy@88eb000 {
                         compatible = "qcom,sm8150-qmp-usb3-uni-phy";
                         reg = <0 0x088eb000 0 0x200>;
                         status = "disabled";
                         #address-cells = <2>;
                         #size-cells = <2>;
                         ranges;

                         clocks = <&gcc GCC_USB3_SEC_PHY_AUX_CLK>,
                                  <&rpmhcc RPMH_CXO_CLK>,
                                  <&gcc GCC_USB3_SEC_CLKREF_CLK>,
                                  <&gcc GCC_USB3_SEC_PHY_COM_AUX_CLK>;
                         clock-names = "aux", "ref_clk_src", "ref", 
"com_aux";

                         resets = <&gcc GCC_USB3PHY_PHY_SEC_BCR>,
                                  <&gcc GCC_USB3_PHY_SEC_BCR>;
                         reset-names = "phy", "common";

                         usb_2_ssphy: phy@88eb200 {
                                 reg = <0 0x088eb200 0 0x200>,
                                       <0 0x088eb400 0 0x200>,
                                       <0 0x088eb800 0 0x800>,
                                       <0 0x088eb600 0 0x200>;
                                 #clock-cells = <0>;
                                 #phy-cells = <0>;
                                 clocks = <&gcc GCC_USB3_SEC_PHY_PIPE_CLK>;
                                 clock-names = "pipe0";
                                 clock-output-names = 
"usb3_uni_phy_pipe_clk_src";
                         };
                 };

IIRC, when I tried using the above sm8150 dt on 5.15.y, the phy_create 
was (either not getting called) or crashing. Probably because 
"of_node_name_eq()" didn't find either "dp-phy" or "usb3-phy" and cfg 
variable was NULL.

I can try reproducing the issue and get back again in a week.

Apologies if I have misunderstood something and this patch doesn't make 
sense. Let me know if I have made any mistake anywhere (either in my DT) 
or in understanding.

Regards,
Krishna,

Re: [PATCH 5.15.y] phy: qcom: qmp: Fix NULL pointer dereference for USB Uni PHYs

[Dmitry Baryshkov]

On Sun, 17 Nov 2024 at 13:51, Krishna Kurapati
<quic_kriskura@quicinc.com> wrote:
>
>
>
> On 11/15/2024 9:29 PM, Dmitry Baryshkov wrote:
> > On Fri, Nov 15, 2024 at 02:45:45PM +0530, Krishna Kurapati wrote:
> >> Commit [1] introduced DP support to QMP driver. While doing so, the
> >> dp and usb configuration structures were added to a combo_phy_cfg
> >> structure. During probe, the match data is used to parse and identify the
> >> dp and usb configs separately. While doing so, the usb_cfg variable
> >> represents the configuration parameters for USB part of the phy (whether
> >> it is DP-Cobo or Uni). during probe, one corner case of parsing usb_cfg
> >> for Uni PHYs is left incomplete and it is left as NULL. This NULL variable
> >> further percolates down to qmp_phy_create() call essentially getting
> >> de-referenced and causing a crash.
> >
> > The UNI PHY platforms don't have usb3-phy subnode. As such the usb_cfg
> > variable should not be used in the for_each_available_child_of_node()
> > loop.
> >
> > Please provide details for the platform on which you observe the crash
> > and the backtrace.
> >
>
> I got this error when I started working on multiport support (begining
> of 2023). Initially I tried testing on my code on 5.15, hence this patch
> was raised on the same.
>
> The 2 qmp phys on sa8195 and sa8295 (based on sc8280xp) are uni phy and
> the following was the DT node that worked out for me on 5.15 codebase:
>
>
>         usb_1_qmpphy: ssphy@88eb000 {
>                 compatible = "qcom,sm8150-qmp-usb3-uni-phy";
>                 reg = <0x088eb000 0x200>;
>                 #address-cells = <1>;
>                 #size-cells = <1>;
>                 ranges;
>                 //status = "disabled";
>                 clocks = <&gcc GCC_USB3_MP_PHY_AUX_CLK>,
>                          <&rpmhcc RPMH_CXO_CLK>,
>                          <&gcc GCC_USB3_SEC_CLKREF_CLK>,
>                          <&gcc GCC_USB3_MP_PHY_COM_AUX_CLK>;
>                 clock-names = "aux", "ref_clk_src", "ref", "com_aux";
>
>                 resets = <&gcc GCC_USB3UNIPHY_PHY_MP0_BCR>,
>                          <&gcc GCC_USB3_UNIPHY_MP0_BCR>;
>                 reset-names = "phy", "common";
>
>                 //vdda-phy-supply = <&L3C>;
>                 vdda-pll-supply = <&L5E>;
>
>                 usb_1_ssphy: usb3-phy@88eb200 {

As this is a UNI PHY and not a combo PHY, the child node should be
just phy@, not usb3-phy@. See
Documentation/devicetree/bindings/phy/qcom,qmp-phy.yaml

>                         reg = <0x088eb200 0x200>,
>                               <0x088eb400 0x200>,
>                               <0x088eb800 0x800>,
>                               <0x088eb600 0x200>;
>                         #clock-cells = <0>;
>                         #phy-cells = <0>;
>                         clocks = <&gcc GCC_USB3_MP_PHY_PIPE_0_CLK>;
>                         clock-names = "pipe0";
>                         clock-output-names = "usb3_uni_phy_pipe_clk_src";
>                 };
>         };
>
>
> I was hitting the bug when I write the DT above way on top of 5.15 baseline.
>
> In 5.15.y, the SM8150 usb_2_qmpphy dT is as follows:
>
>                  usb_2_qmpphy: phy@88eb000 {
>                          compatible = "qcom,sm8150-qmp-usb3-uni-phy";
>                          reg = <0 0x088eb000 0 0x200>;
>                          status = "disabled";
>                          #address-cells = <2>;
>                          #size-cells = <2>;
>                          ranges;
>
>                          clocks = <&gcc GCC_USB3_SEC_PHY_AUX_CLK>,
>                                   <&rpmhcc RPMH_CXO_CLK>,
>                                   <&gcc GCC_USB3_SEC_CLKREF_CLK>,
>                                   <&gcc GCC_USB3_SEC_PHY_COM_AUX_CLK>;
>                          clock-names = "aux", "ref_clk_src", "ref",
> "com_aux";
>
>                          resets = <&gcc GCC_USB3PHY_PHY_SEC_BCR>,
>                                   <&gcc GCC_USB3_PHY_SEC_BCR>;
>                          reset-names = "phy", "common";
>
>                          usb_2_ssphy: phy@88eb200 {

Just as I wrote, this one correctly uses phy@

>                                  reg = <0 0x088eb200 0 0x200>,
>                                        <0 0x088eb400 0 0x200>,
>                                        <0 0x088eb800 0 0x800>,
>                                        <0 0x088eb600 0 0x200>;
>                                  #clock-cells = <0>;
>                                  #phy-cells = <0>;
>                                  clocks = <&gcc GCC_USB3_SEC_PHY_PIPE_CLK>;
>                                  clock-names = "pipe0";
>                                  clock-output-names =
> "usb3_uni_phy_pipe_clk_src";
>                          };
>                  };
>
> IIRC, when I tried using the above sm8150 dt on 5.15.y, the phy_create
> was (either not getting called) or crashing. Probably because
> "of_node_name_eq()" didn't find either "dp-phy" or "usb3-phy" and cfg
> variable was NULL.

Unless somebody backported some patch in an incorrect way, the SM8150
DT entry is correct, while SA8xxx is not,

>
> I can try reproducing the issue and get back again in a week.

Yes, please.

>
> Apologies if I have misunderstood something and this patch doesn't make
> sense. Let me know if I have made any mistake anywhere (either in my DT)
> or in understanding.
>
> Regards,
> Krishna,



-- 
With best wishes
Dmitry