| Message ID | 20241115091545.2358156-1-quic_kriskura@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [5.15.y] phy: qcom: qmp: Fix NULL pointer dereference for USB Uni PHYs | expand |
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 >
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,
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,
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 */
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(+)