diff mbox series

PCI: rockchip-dwc: Potential error pointer dereference in probe

Message ID 20210813113338.GA30697@kili (mailing list archive)
State New, archived
Headers show
Series PCI: rockchip-dwc: Potential error pointer dereference in probe | expand

Commit Message

Dan Carpenter Aug. 13, 2021, 11:33 a.m. UTC
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(-)

Comments

Robin Murphy Aug. 13, 2021, 12:55 p.m. UTC | #1
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) {
>
Rob Herring (Arm) Aug. 13, 2021, 1:34 p.m. UTC | #2
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) {
> >
Robin Murphy Aug. 13, 2021, 1:47 p.m. UTC | #3
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) {
>>>
Dan Carpenter Aug. 13, 2021, 1:54 p.m. UTC | #4
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
Robin Murphy Aug. 13, 2021, 2:01 p.m. UTC | #5
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.
Dan Carpenter Aug. 13, 2021, 2:12 p.m. UTC | #6
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
Mark Brown Aug. 13, 2021, 2:32 p.m. UTC | #7
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.
Robin Murphy Aug. 13, 2021, 3 p.m. UTC | #8
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.
Mark Brown Aug. 13, 2021, 3:30 p.m. UTC | #9
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 :(
Dan Carpenter Aug. 13, 2021, 3:45 p.m. UTC | #10
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
Mark Brown Aug. 13, 2021, 3:53 p.m. UTC | #11
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...
Rob Herring (Arm) Aug. 13, 2021, 3:57 p.m. UTC | #12
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... ;)
Mark Brown Aug. 13, 2021, 4:01 p.m. UTC | #13
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 mbox series

Patch

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) {