| Message ID | 20210813113338.GA30697@kili (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: rockchip-dwc: Potential error pointer dereference in probe | expand |
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. > Fixes: e1229e884e19 ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > 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) { >
On Fri, Aug 13, 2021 at 7:55 AM Robin Murphy <robin.murphy@arm.com> wrote: > > 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? I'm pretty sure 'optional' means ENODEV is never returned. So there wasn't any real problem, but the check was unnecessary. > > Robin. > > > Fixes: e1229e884e19 ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > --- > > 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) { > >
On 2021-08-13 14:34, Rob Herring wrote: > On Fri, Aug 13, 2021 at 7:55 AM Robin Murphy <robin.murphy@arm.com> wrote: >> >> 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? > > 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. >>> Fixes: e1229e884e19 ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver") >>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> >>> --- >>> 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) { >>>
On Fri, Aug 13, 2021 at 01:55:38PM +0100, Robin Murphy wrote: > 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? 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
On 2021-08-13 14:54, Dan Carpenter wrote: > On Fri, Aug 13, 2021 at 01:55:38PM +0100, Robin Murphy wrote: >> 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? > > 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.
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
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: > >> 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.
On Fri, Aug 13, 2021 at 04:00:45PM +0100, Robin Murphy wrote: > On 2021-08-13 15:32, Mark Brown wrote: > > 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: > > > 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
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: > > 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 > 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 <robin.murphy@arm.com> wrote: > > On 2021-08-13 14:34, Rob Herring wrote: > > On Fri, Aug 13, 2021 at 7:55 AM Robin Murphy <robin.murphy@arm.com> wrote: > >> > >> 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? > > > > 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... ;)
On Fri, Aug 13, 2021 at 10:57:44AM -0500, Rob Herring wrote: > On Fri, Aug 13, 2021 at 8:47 AM Robin Murphy <robin.murphy@arm.com> wrote: > > 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!
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) {
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 <dan.carpenter@oracle.com> --- drivers/pci/controller/dwc/pcie-dw-rockchip.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)