Message ID | 20250413212518.2625540-1-chenyuan0y@gmail.com |
---|---|
State | Superseded |
Headers | show |
Series | phy: qcom-qmp-usb: Fix an NULL vs IS_ERR() bug | expand |
On Sun, Apr 13, 2025 at 04:25:18PM -0500, Chenyuan Yang wrote: > In qmp_usb_iomap(), one branch returns the result of devm_ioremap(), which > can be NULL. Since IS_ERR() does not catch a NULL pointer, > add an explicit NULL check in qmp_usb_parse_dt_legacy() to prevent > potential dereference issues. Good catch, but please move the handling of this into the qmp_usb_iomap() helper so that it returns an error pointer also if devm_ioremap() fails. > Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com> > Fixes: 2a55ec4f0a04 ("phy: qcom-qmp-usb: merge driver data") This is not the commit that introduced the issue; this should be: Fixes: a5d6b1ac56cb ("phy: qcom-qmp-usb: fix memleak on probe deferral") > qmp->pcs = qmp_usb_iomap(dev, np, 2, exclusive); > + if (!qmp->pcs) > + return -ENOMEM; > if (IS_ERR(qmp->pcs)) > return PTR_ERR(qmp->pcs); Johan
On 13/04/2025 23:25, Chenyuan Yang wrote: > In qmp_usb_iomap(), one branch returns the result of devm_ioremap(), which > can be NULL. Since IS_ERR() does not catch a NULL pointer, No, that's not true. NAK. Best regards, Krzysztof
On Mon, Apr 14, 2025 at 09:30:19AM +0200, Krzysztof Kozlowski wrote: > On 13/04/2025 23:25, Chenyuan Yang wrote: > > In qmp_usb_iomap(), one branch returns the result of devm_ioremap(), which > > can be NULL. Since IS_ERR() does not catch a NULL pointer, > > No, that's not true. NAK. I'm afraid you're mistaken here. See __devm_ioremap() which can return NULL. Johan
On 14/04/2025 09:40, Johan Hovold wrote: > On Mon, Apr 14, 2025 at 09:30:19AM +0200, Krzysztof Kozlowski wrote: >> On 13/04/2025 23:25, Chenyuan Yang wrote: >>> In qmp_usb_iomap(), one branch returns the result of devm_ioremap(), which >>> can be NULL. Since IS_ERR() does not catch a NULL pointer, >> >> No, that's not true. NAK. > > I'm afraid you're mistaken here. See __devm_ioremap() which can return > NULL. > Uh, you are right, I only checked devm_of_iomap in qmp_usb_iomap(). Anyway, the fix should be different - given function should either return ERR or NULL, not both, so devm_ioremap return value needs to be wrapped in ERR_PTR. Best regards, Krzysztof
On Mon, Apr 14, 2025 at 10:08:18AM +0200, Krzysztof Kozlowski wrote: > On 14/04/2025 09:40, Johan Hovold wrote: > > I'm afraid you're mistaken here. See __devm_ioremap() which can return > > NULL. > > > Uh, you are right, I only checked devm_of_iomap in qmp_usb_iomap(). > Anyway, the fix should be different - given function should either > return ERR or NULL, not both, so devm_ioremap return value needs to be > wrapped in ERR_PTR. Right, I already suggested that: https://lore.kernel.org/lkml/Z_yxxoa12N9rNn2z@hovoldconsulting.com/ Johan
Hi Johan and Krzysztof, On Mon, Apr 14, 2025 at 3:13 AM Johan Hovold <johan@kernel.org> wrote: > > On Mon, Apr 14, 2025 at 10:08:18AM +0200, Krzysztof Kozlowski wrote: > > On 14/04/2025 09:40, Johan Hovold wrote: > > > > I'm afraid you're mistaken here. See __devm_ioremap() which can return > > > NULL. > > > > > Uh, you are right, I only checked devm_of_iomap in qmp_usb_iomap(). > > Anyway, the fix should be different - given function should either > > return ERR or NULL, not both, so devm_ioremap return value needs to be > > wrapped in ERR_PTR. > > Right, I already suggested that: > > https://lore.kernel.org/lkml/Z_yxxoa12N9rNn2z@hovoldconsulting.com/ > > Johan I have submitted "[PATCH v2] phy: qcom-qmp-usb: Fix an NULL vs IS_ERR() bug", which fixes this issue based on your suggestions -Chenyuan
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-usb.c b/drivers/phy/qualcomm/phy-qcom-qmp-usb.c index 787721570457..8dab20b0c11c 100644 --- a/drivers/phy/qualcomm/phy-qcom-qmp-usb.c +++ b/drivers/phy/qualcomm/phy-qcom-qmp-usb.c @@ -2152,6 +2152,8 @@ static int qmp_usb_parse_dt_legacy(struct qmp_usb *qmp, struct device_node *np) return PTR_ERR(qmp->rx); qmp->pcs = qmp_usb_iomap(dev, np, 2, exclusive); + if (!qmp->pcs) + return -ENOMEM; if (IS_ERR(qmp->pcs)) return PTR_ERR(qmp->pcs);
In qmp_usb_iomap(), one branch returns the result of devm_ioremap(), which can be NULL. Since IS_ERR() does not catch a NULL pointer, add an explicit NULL check in qmp_usb_parse_dt_legacy() to prevent potential dereference issues. Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com> Fixes: 2a55ec4f0a04 ("phy: qcom-qmp-usb: merge driver data") --- drivers/phy/qualcomm/phy-qcom-qmp-usb.c | 2 ++ 1 file changed, 2 insertions(+)