PCI: rockchip-dwc: Potential error pointer dereference in probe
PATCH 1/1 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.
--- drivers/pci/controller/dwc/pcie-dw-rockchip.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/pci/controller/dwc/pcie-dw-rockchip.c +2 -3
@@ -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) {
Signature · 1 line
2.20.1
Quoted 4 lines · Dan Carpenter
If
devm_regulator_get_optional()returns an error pointer, then we should return it to the user. The current code makes an exception for-ENODEVthat will result in an error pointer dereference on the next line when it callsregulator_enable(). Remove the exception.
Doesn't this break the apparent intent of the regulator being optional, though?
Robin.
Quoted 23 lines
Fixes e1229e884e19 ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")Signed-off-by Dan Carpenter <[email protected]>--- drivers/pci/controller/dwc/pcie-dw-rockchip.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)drivers/pci/controller/dwc/pcie-dw-rockchip.c +2
-3@@ -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) {
Rob Herring
reviewerQuoted 9 lines · Robin Murphy
Quoted 4 lines · Dan Carpenter
If
devm_regulator_get_optional()returns an error pointer, then we should return it to the user. The current code makes an exception for-ENODEVthat will result in an error pointer dereference on the next line when it callsregulator_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.
Quoted 23 lines
Robin.
Quoted 20 lines
Fixes e1229e884e19 ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")Signed-off-by Dan Carpenter <[email protected]>--- drivers/pci/controller/dwc/pcie-dw-rockchip.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)drivers/pci/controller/dwc/pcie-dw-rockchip.c +2
-3@@ -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" );
Quoted 3 lines
drivers/pci/controller/dwc/pcie-dw-rockchip.c static int rockchip_pcie_probe(struct platform_device *pdev) · line 232
Quoted 3 lines
drivers/pci/controller/dwc/pcie-dw-rockchip.c +0
-0ret =regulator_enable (rockchip->vpcie3v3);if (ret) {
_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip
Quoted 8 lines · Rob Herring
Quoted 7 lines · Robin Murphy
Quoted 4 lines · Dan Carpenter
If
devm_regulator_get_optional()returns an error pointer, then we should return it to the user. The current code makes an exception for-ENODEVthat will result in an error pointer dereference on the next line when it callsregulator_enable(). Remove the exception.
Quoted 5 lines
Quoted 2 lines
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 23 lines
Quoted 23 lines
Quoted 23 lines
Fixes e1229e884e19 ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")Signed-off-by Dan Carpenter <[email protected]>--- drivers/pci/controller/dwc/pcie-dw-rockchip.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)drivers/pci/controller/dwc/pcie-dw-rockchip.c +2
-3@@ -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) {
Rob Herring
reviewerQuoted 16 lines · Robin Murphy
Quoted 8 lines · Rob Herring
Quoted 7 lines · Robin Murphy
Quoted 4 lines · Dan Carpenter
If
devm_regulator_get_optional()returns an error pointer, then we should return it to the user. The current code makes an exception for-ENODEVthat will result in an error pointer dereference on the next line when it callsregulator_enable(). Remove the exception.Quoted 5 lines
Quoted 2 lines
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.
Quoted 6 lines
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
-ENODEVif 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... ;)
Quoted 11 lines · Rob Herring
Quoted 6 lines · Robin Murphy
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
-ENODEVif 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!
Quoted 8 lines · Robin Murphy
Quoted 4 lines · Dan Carpenter
If
devm_regulator_get_optional()returns an error pointer, then we should return it to the user. The current code makes an exception for-ENODEVthat will result in an error pointer dereference on the next line when it callsregulator_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
Quoted 11 lines · Dan Carpenter
Quoted 8 lines · Robin Murphy
Quoted 4 lines · Dan Carpenter
If
devm_regulator_get_optional()returns an error pointer, then we should return it to the user. The current code makes an exception for-ENODEVthat will result in an error pointer dereference on the next line when it callsregulator_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.
Quoted 11 lines
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-ENODEVwhich 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.
Quoted 3 lines · Robin Murphy
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
PATCH v2 1/1 PCI: rockchip-dwc: Potential error pointer dereference in probe
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.
---
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(-)
drivers/pci/controller/dwc/pcie-dw-rockchip.c +10 -7
@@ -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; }
Signature · 1 line
2.20.1
Quoted 4 lines · Dan Carpenter
If
devm_regulator_get_optional()returns-ENODEVthen it would lead to an error pointer dereference on the next line and in the error handling code.
Quoted 8 lines
Fixes e1229e884e19 ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")Signed-off-by Dan Carpenter <[email protected]>---
v2: The-ENODEVfromdevm_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
Quoted 41 lines
drivers/pci/controller/dwc/pcie-dw-rockchip.c +10
-7@@ -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; }Signature · 1 line
2.20.1
Quoted 3 lines · Robin Murphy
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.
Quoted 6 lines · Mark Brown
Quoted 3 lines · Robin Murphy
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 7 lines
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.
Quoted 10 lines · Robin Murphy
Quoted 7 lines · Mark Brown
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.
Quoted 4 lines
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.
Quoted 5 lines
A lot of the usage of
regulator_get_optional()seems to be just making sure some external thing is powered betweenprobe()andremove()if it's not hard-wired already, so maybe something like adevm_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 :(
Quoted 7 lines · Mark Brown
Quoted 3 lines · Robin Murphy
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
Quoted 11 lines · Dan Carpenter
Quoted 7 lines · Mark Brown
Quoted 3 lines · Robin Murphy
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?
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.
Quoted 7 lines
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...