[PATCH] PCI: rockchip-dwc: Potential error pointer dereference in probe

If devm_regulator_get_optional() returns an error pointer, then we
should return it to the user.  The current code makes an exception
for -ENODEV that will result in an error pointer dereference on the
next line when it calls regulator_enable().  Remove the exception.

Fixes: e1229e884e19 ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")
Signed-off-by: Dan Carpenter <redacted>
---
 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index 20cef2e06f66..2d0ffd3c4e16 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c

@@ -225,9 +225,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
 	/* DON'T MOVE ME: must be enable before PHY init */
 	rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
 	if (IS_ERR(rockchip->vpcie3v3))
-		if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV)
-			return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
-					"failed to get vpcie3v3 regulator\n");
+		return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
+				     "failed to get vpcie3v3 regulator\n");
 
 	ret = regulator_enable(rockchip->vpcie3v3);
 	if (ret) {

-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On 2021-08-13 12:33, Dan Carpenter wrote:

If devm_regulator_get_optional() returns an error pointer, then we
should return it to the user.  The current code makes an exception
for -ENODEV that will result in an error pointer dereference on the
next line when it calls regulator_enable().  Remove the exception.

Doesn't this break the apparent intent of the regulator being optional, 
though?

Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Fri, Aug 13, 2021 at 7:55 AM Robin Murphy [off-list ref] wrote:

On 2021-08-13 12:33, Dan Carpenter wrote:

quoted

If devm_regulator_get_optional() returns an error pointer, then we
should return it to the user.  The current code makes an exception
for -ENODEV that will result in an error pointer dereference on the
next line when it calls regulator_enable().  Remove the exception.

Doesn't this break the apparent intent of the regulator being optional,
though?

I'm pretty sure 'optional' means ENODEV is never returned. So there
wasn't any real problem, but the check was unnecessary.

Robin.

quoted

Fixes: e1229e884e19 ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")
Signed-off-by: Dan Carpenter <redacted>
---
  drivers/pci/controller/dwc/pcie-dw-rockchip.c | 5 ++---
  1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index 20cef2e06f66..2d0ffd3c4e16 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c

@@ -225,9 +225,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
      /* DON'T MOVE ME: must be enable before PHY init */
      rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
      if (IS_ERR(rockchip->vpcie3v3))
-             if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV)
-                     return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
-                                     "failed to get vpcie3v3 regulator\n");
+             return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
+                                  "failed to get vpcie3v3 regulator\n");

      ret = regulator_enable(rockchip->vpcie3v3);
      if (ret) {

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On 2021-08-13 14:34, Rob Herring wrote:

On Fri, Aug 13, 2021 at 7:55 AM Robin Murphy [off-list ref] wrote:

quoted

On 2021-08-13 12:33, Dan Carpenter wrote:

quoted

If devm_regulator_get_optional() returns an error pointer, then we
should return it to the user.  The current code makes an exception
for -ENODEV that will result in an error pointer dereference on the
next line when it calls regulator_enable().  Remove the exception.

Doesn't this break the apparent intent of the regulator being optional,
though?

I'm pretty sure 'optional' means ENODEV is never returned. So there
wasn't any real problem, but the check was unnecessary.

In fact it's the other way round - "optional" in this case is for when 
the supply may legitimately not exist so the driver may or may not need 
to handle it, so it can return -ENODEV if a regulator isn't described by 
firmware. A non-optional regulator is assumed to represent a necessary 
supply, so if there's nothing described by firmware you get the (valid) 
dummy regulator back.

Robin.

quoted

quoted

Fixes: e1229e884e19 ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")
Signed-off-by: Dan Carpenter <redacted>
---
   drivers/pci/controller/dwc/pcie-dw-rockchip.c | 5 ++---
   1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index 20cef2e06f66..2d0ffd3c4e16 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c

@@ -225,9 +225,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
       /* DON'T MOVE ME: must be enable before PHY init */
       rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
       if (IS_ERR(rockchip->vpcie3v3))
-             if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV)
-                     return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
-                                     "failed to get vpcie3v3 regulator\n");
+             return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
+                                  "failed to get vpcie3v3 regulator\n");

       ret = regulator_enable(rockchip->vpcie3v3);
       if (ret) {

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Fri, Aug 13, 2021 at 01:55:38PM +0100, Robin Murphy wrote:

On 2021-08-13 12:33, Dan Carpenter wrote:

quoted

If devm_regulator_get_optional() returns an error pointer, then we
should return it to the user.  The current code makes an exception
for -ENODEV that will result in an error pointer dereference on the
next line when it calls regulator_enable().  Remove the exception.

Doesn't this break the apparent intent of the regulator being optional,
though?

Argh...  Crap.  My patch is wrong, but the bug is real.

This code should follow the standard kernel idiom of returning error
pointers when there are errors and returning NULL when an optional
feature is disabled.  The problem with returning a Special Error code
to mean "disabled" is that someone will use the Special Error code to
mean an error.

And that has already sort of happened, because _regulator_get() returns
-ENODEV which would will cause the Oops I described in my patch.

Ugh...  The correct thing is to convert it to NULL/error pointers.  I
have not looked at how hard that is though...

regards,
dan carpenter



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On 2021-08-13 14:54, Dan Carpenter wrote:

On Fri, Aug 13, 2021 at 01:55:38PM +0100, Robin Murphy wrote:

quoted

On 2021-08-13 12:33, Dan Carpenter wrote:

quoted

If devm_regulator_get_optional() returns an error pointer, then we
should return it to the user.  The current code makes an exception
for -ENODEV that will result in an error pointer dereference on the
next line when it calls regulator_enable().  Remove the exception.

Doesn't this break the apparent intent of the regulator being optional,
though?

Argh...  Crap.  My patch is wrong, but the bug is real.

Yeah, I guess this probably wants to follow the same pattern as 
rockchip_pcie_set_vpcie() in the older driver, where it's the regulator 
API calls which get wrapped in checks.

This code should follow the standard kernel idiom of returning error
pointers when there are errors and returning NULL when an optional
feature is disabled.  The problem with returning a Special Error code
to mean "disabled" is that someone will use the Special Error code to
mean an error.

And that has already sort of happened, because _regulator_get() returns
-ENODEV which would will cause the Oops I described in my patch.

Ugh...  The correct thing is to convert it to NULL/error pointers.  I
have not looked at how hard that is though...

Indeed I've thought before that it would be nice if regulators worked 
like GPIOs, where the absence of an optional one does give you NULL, and 
most of the API is also NULL-safe. Probably a pretty big job though...

Thanks,
Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Fri, Aug 13, 2021 at 03:01:10PM +0100, Robin Murphy wrote:

Indeed I've thought before that it would be nice if regulators worked like
GPIOs, where the absence of an optional one does give you NULL, and most of
the API is also NULL-safe. Probably a pretty big job though...

Yeah.  You have to write a NULL-safe API with optional feature.  And,
yeah, it's a lot of work to do it in retroactively...  Too much.  I'll
just redo my patch.

regards,
dan carpenter

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

If devm_regulator_get_optional() returns -ENODEV then it would lead
to an error pointer dereference on the next line and in the error
handling code.

Fixes: e1229e884e19 ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")
Signed-off-by: Dan Carpenter <redacted>
---
v2: The -ENODEV from devm_regulator_get_optional() has to be handled
    specially.

 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index 20cef2e06f66..c9b341e55cbb 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c

@@ -224,15 +224,17 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
 
 	/* DON'T MOVE ME: must be enable before PHY init */
 	rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
-	if (IS_ERR(rockchip->vpcie3v3))
+	if (IS_ERR(rockchip->vpcie3v3)) {
 		if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV)
 			return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3),
 					"failed to get vpcie3v3 regulator\n");
-
-	ret = regulator_enable(rockchip->vpcie3v3);
-	if (ret) {
-		dev_err(dev, "failed to enable vpcie3v3 regulator\n");
-		return ret;
+		rockchip->vpcie3v3 = NULL;
+	} else {
+		ret = regulator_enable(rockchip->vpcie3v3);
+		if (ret) {
+			dev_err(dev, "failed to enable vpcie3v3 regulator\n");
+			return ret;
+		}
 	}
 
 	ret = rockchip_pcie_phy_init(rockchip);

@@ -255,7 +257,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
 deinit_phy:
 	rockchip_pcie_phy_deinit(rockchip);
 disable_regulator:
-	regulator_disable(rockchip->vpcie3v3);
+	if (rockchip->vpcie3v3)
+		regulator_disable(rockchip->vpcie3v3);
 
 	return ret;
 }

-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Fri, Aug 13, 2021 at 03:01:10PM +0100, Robin Murphy wrote:

Indeed I've thought before that it would be nice if regulators worked like
GPIOs, where the absence of an optional one does give you NULL, and most of
the API is also NULL-safe. Probably a pretty big job though...

It also encourages *really* bad practice with error handling, and in
general there are few use cases for optional regulators where there's
not some other actions that need to be taken in the case where the
supply isn't there (elimintating some operating points or features,
reconfiguring power internally and so on).  If we genuninely don't need
to do anything special one wonders why we're trying to turn the power on
in the first place.

On 2021-08-13 15:32, Mark Brown wrote:

On Fri, Aug 13, 2021 at 03:01:10PM +0100, Robin Murphy wrote:

quoted

Indeed I've thought before that it would be nice if regulators worked like
GPIOs, where the absence of an optional one does give you NULL, and most of
the API is also NULL-safe. Probably a pretty big job though...

It also encourages *really* bad practice with error handling, and in
general there are few use cases for optional regulators where there's
not some other actions that need to be taken in the case where the
supply isn't there (elimintating some operating points or features,
reconfiguring power internally and so on).  If we genuninely don't need
to do anything special one wonders why we're trying to turn the power on
in the first place.

Sure, once you get into it, regulators are arguably a rather deeper area 
than GPIOs, so in terms of the NULL-safe aspect anything beyond 
enable/disable - for the sake of keeping trivial usage simple - would be 
pretty questionable for sure.

A lot of the usage of regulator_get_optional() seems to be just making 
sure some external thing is powered between probe() and remove() if it's 
not hard-wired already, so maybe something like a 
devm_regulator_get_optional_enabled() could be an answer to that 
argument without even touching the underlying API.

Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Fri, Aug 13, 2021 at 04:00:45PM +0100, Robin Murphy wrote:

On 2021-08-13 15:32, Mark Brown wrote:

quoted

It also encourages *really* bad practice with error handling, and in
general there are few use cases for optional regulators where there's
not some other actions that need to be taken in the case where the
supply isn't there (elimintating some operating points or features,
reconfiguring power internally and so on).  If we genuninely don't need
to do anything special one wonders why we're trying to turn the power on
in the first place.

Sure, once you get into it, regulators are arguably a rather deeper area
than GPIOs, so in terms of the NULL-safe aspect anything beyond
enable/disable - for the sake of keeping trivial usage simple - would be
pretty questionable for sure.

enable and disable are among my biggest worries frankly, if the device
was supposed to have power and that didn't work then there's probably
some kind of issue.

A lot of the usage of regulator_get_optional() seems to be just making sure
some external thing is powered between probe() and remove() if it's not
hard-wired already, so maybe something like a
devm_regulator_get_optional_enabled() could be an answer to that argument
without even touching the underlying API.

I'm fairly sure a lot of the regulator_get_optional() usage is just
abuse of the API, every so often I get fed up enough to send patches
converting users that look like you describe to normal regulator API
calls.  People really don't get the dummy regulator stuff and seem to
think that _get_optional() is what you use to avoid writing any error
handling code :(

On Fri, Aug 13, 2021 at 03:32:50PM +0100, Mark Brown wrote:

On Fri, Aug 13, 2021 at 03:01:10PM +0100, Robin Murphy wrote:

quoted

Indeed I've thought before that it would be nice if regulators worked like
GPIOs, where the absence of an optional one does give you NULL, and most of
the API is also NULL-safe. Probably a pretty big job though...

It also encourages *really* bad practice with error handling

I'm not necessarily 100% positive what you mean by this.  I think you
mean you don't like when people pass invalid pointers to free functions?
But making regulator code NULL-safe wouldn't affect error handling
because NULL wouldn't be an error.

	p = get_optional();
	if (IS_ERR(p))
		return PTR_ERR(p);
	enable(p);
	...
	disable(p);

It all works nicely.

regards,
dan carpenter


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Fri, Aug 13, 2021 at 06:45:05PM +0300, Dan Carpenter wrote:

On Fri, Aug 13, 2021 at 03:32:50PM +0100, Mark Brown wrote:

quoted

On Fri, Aug 13, 2021 at 03:01:10PM +0100, Robin Murphy wrote:

quoted

quoted

Indeed I've thought before that it would be nice if regulators worked like
GPIOs, where the absence of an optional one does give you NULL, and most of
the API is also NULL-safe. Probably a pretty big job though...

quoted

It also encourages *really* bad practice with error handling

I'm not necessarily 100% positive what you mean by this.  I think you
mean you don't like when people pass invalid pointers to free functions?

No, it's the case where people don't bother checking if they got the
regulator in the first place, don't bother checking if when they tried
to enable the regulator that actually worked, and don't do whatever
extra handling they need to do to configure the system for the fact that
one of the power supplies is missing.  It really only helps in the case
where you can just ignore the regulator completely.

But making regulator code NULL-safe wouldn't affect error handling
because NULL wouldn't be an error.

	p = get_optional();
	if (IS_ERR(p))
		return PTR_ERR(p);
	enable(p);

Your example already misses the error handling on enable...

On Fri, Aug 13, 2021 at 8:47 AM Robin Murphy [off-list ref] wrote:

On 2021-08-13 14:34, Rob Herring wrote:

quoted

On Fri, Aug 13, 2021 at 7:55 AM Robin Murphy [off-list ref] wrote:

quoted

On 2021-08-13 12:33, Dan Carpenter wrote:

quoted

If devm_regulator_get_optional() returns an error pointer, then we
should return it to the user.  The current code makes an exception
for -ENODEV that will result in an error pointer dereference on the
next line when it calls regulator_enable().  Remove the exception.

Doesn't this break the apparent intent of the regulator being optional,
though?

I'm pretty sure 'optional' means ENODEV is never returned. So there
wasn't any real problem, but the check was unnecessary.

In fact it's the other way round - "optional" in this case is for when
the supply may legitimately not exist so the driver may or may not need
to handle it, so it can return -ENODEV if a regulator isn't described by
firmware. A non-optional regulator is assumed to represent a necessary
supply, so if there's nothing described by firmware you get the (valid)
dummy regulator back.

Ah yes, regulators is the oddball. Surely no one else will assume the
same behavior of _optional() variants across subsystems... ;)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Fri, Aug 13, 2021 at 10:57:44AM -0500, Rob Herring wrote:

On Fri, Aug 13, 2021 at 8:47 AM Robin Murphy [off-list ref] wrote:

quoted

In fact it's the other way round - "optional" in this case is for when
the supply may legitimately not exist so the driver may or may not need
to handle it, so it can return -ENODEV if a regulator isn't described by
firmware. A non-optional regulator is assumed to represent a necessary
supply, so if there's nothing described by firmware you get the (valid)
dummy regulator back.

Ah yes, regulators is the oddball. Surely no one else will assume the
same behavior of _optional() variants across subsystems... ;)

It would be silly to copy the *whole* pattern!

On Fri, Aug 13, 2021 at 05:26:48PM +0300, Dan Carpenter wrote:

If devm_regulator_get_optional() returns -ENODEV then it would lead
to an error pointer dereference on the next line and in the error
handling code.

Fixes: e1229e884e19 ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")
Signed-off-by: Dan Carpenter <redacted>
---
v2: The -ENODEV from devm_regulator_get_optional() has to be handled
    specially.

 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

I squashed it in with the patch that is fixing, thanks a lot.

Lorenzo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel