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(+)

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);
 
-- 
2.34.1

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

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