[PATCH] pinctrl: intel: fix NULL pointer deref

[PATCH] pinctrl: intel: fix NULL pointer deref

[Henning Schild]

match could be NULL in which case we do not go ACPI after all

Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 drivers/pinctrl/intel/pinctrl-intel.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 85750974d182..dca17bb76cac 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -1601,12 +1601,12 @@ const struct intel_pinctrl_soc_data *intel_pinctrl_get_soc_data(struct platform_
 	const struct intel_pinctrl_soc_data *data = NULL;
 	const struct intel_pinctrl_soc_data **table;
 	struct acpi_device *adev;
+	const void *match;
 	unsigned int i;
 
 	adev = ACPI_COMPANION(&pdev->dev);
-	if (adev) {
-		const void *match = device_get_match_data(&pdev->dev);
-
+	match = device_get_match_data(&pdev->dev);
+	if (adev && match) {
 		table = (const struct intel_pinctrl_soc_data **)match;
 		for (i = 0; table[i]; i++) {
 			if (!strcmp(adev->pnp.unique_id, table[i]->uid)) {
-- 
2.31.1

Re: [PATCH] pinctrl: intel: fix NULL pointer deref

[Mika Westerberg]

Hi,

On Wed, Jun 09, 2021 at 08:27:22AM +0200, Henning Schild wrote:
> match could be NULL in which case we do not go ACPI after all
> 
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---
>  drivers/pinctrl/intel/pinctrl-intel.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index 85750974d182..dca17bb76cac 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -1601,12 +1601,12 @@ const struct intel_pinctrl_soc_data *intel_pinctrl_get_soc_data(struct platform_
>  	const struct intel_pinctrl_soc_data *data = NULL;
>  	const struct intel_pinctrl_soc_data **table;
>  	struct acpi_device *adev;
> +	const void *match;
>  	unsigned int i;
>  
>  	adev = ACPI_COMPANION(&pdev->dev);
> -	if (adev) {
> -		const void *match = device_get_match_data(&pdev->dev);
> -
> +	match = device_get_match_data(&pdev->dev);

Actually we don't even call intel_pinctrl_get_soc_data() if the ACPI ID
is not listed in the corresponding driver's module table. So I don't
think match can ever be NULL.

But feel free to prove me wrong ;-)

Re: [PATCH] pinctrl: intel: fix NULL pointer deref

[Andy Shevchenko]

On Wed, Jun 9, 2021 at 1:12 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Wed, Jun 09, 2021 at 08:27:22AM +0200, Henning Schild wrote:
> > match could be NULL in which case we do not go ACPI after all

...

> >       adev = ACPI_COMPANION(&pdev->dev);
> > -     if (adev) {
> > -             const void *match = device_get_match_data(&pdev->dev);
> > -
> > +     match = device_get_match_data(&pdev->dev);
>
> Actually we don't even call intel_pinctrl_get_soc_data() if the ACPI ID
> is not listed in the corresponding driver's module table. So I don't
> think match can ever be NULL.
>
> But feel free to prove me wrong ;-)

It's possible to have bugs in this driver, but can we see the real case here?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] pinctrl: intel: fix NULL pointer deref

[Henning Schild]

Am Wed, 9 Jun 2021 13:33:34 +0300
schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

> On Wed, Jun 9, 2021 at 1:12 PM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Wed, Jun 09, 2021 at 08:27:22AM +0200, Henning Schild wrote:  
> > > match could be NULL in which case we do not go ACPI after all  
> 
> ...
> 
> > >       adev = ACPI_COMPANION(&pdev->dev);
> > > -     if (adev) {
> > > -             const void *match =
> > > device_get_match_data(&pdev->dev); -
> > > +     match = device_get_match_data(&pdev->dev);  
> >
> > Actually we don't even call intel_pinctrl_get_soc_data() if the
> > ACPI ID is not listed in the corresponding driver's module table.
> > So I don't think match can ever be NULL.
> >
> > But feel free to prove me wrong ;-)  
> 
> It's possible to have bugs in this driver, but can we see the real
> case here?

Yes that is indeed only showing when using a kernel that has seen other
patches. To be precise i applied "[rfc, PATCH v1 0/7] PCI: introduce
p2sb helper" before running into the problem. Something in there must
be calling the function without the ACPI ID.

I am still working on a series of device drivers for Siemens PCs,
adding i.e. LEDs which are in fact GPIO. Those PCs have a hidden p2sb
and no ACPI entries for the LEDs.

In order to use GPIO from the drivers i need to make sure
"broxton-pinctrl" comes up even if p2sb is hidden.

Long story short, i thought the patch was simple enough to merge even
taken out of my special context.

Currently intel_pinctl only works if "ps2b is not hidden by BIOS" or
"ACPI tables are correct", lifting the ban on the hidden p2sb seems
like a useful thing in general (i.e. sysfs gpio interface). And i was
hoping Andy would take the lead on that. It is something my Siemens
drivers would depend on, but really a generic thing as far as i
understand it.

regards,
Henning

Re: [PATCH] pinctrl: intel: fix NULL pointer deref

[Andy Shevchenko]

On Wed, Jun 09, 2021 at 01:08:16PM +0200, Henning Schild wrote:
> Am Wed, 9 Jun 2021 13:33:34 +0300
> schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

...

> In order to use GPIO from the drivers i need to make sure
> "broxton-pinctrl" comes up even if p2sb is hidden.
> 
> Long story short, i thought the patch was simple enough to merge even
> taken out of my special context.
> 
> Currently intel_pinctl only works if "ps2b is not hidden by BIOS" or
> "ACPI tables are correct", lifting the ban on the hidden p2sb seems
> like a useful thing in general (i.e. sysfs gpio interface). And i was
> hoping Andy would take the lead on that. It is something my Siemens
> drivers would depend on, but really a generic thing as far as i
> understand it.

From p2sb series discussion it appears that this patch is not needed.
The case is when BIOS already provides an ACPI device.

So, the initial bug is in that series that needs to check if the ACPI device is
exposed and forbid platform device instantiation in that case.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] pinctrl: intel: fix NULL pointer deref

[Andy Shevchenko]

On Thu, Jun 10, 2021 at 05:25:04PM +0300, Andy Shevchenko wrote:
> On Wed, Jun 09, 2021 at 01:08:16PM +0200, Henning Schild wrote:
> > Am Wed, 9 Jun 2021 13:33:34 +0300
> > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> 
> ...
> 
> > In order to use GPIO from the drivers i need to make sure
> > "broxton-pinctrl" comes up even if p2sb is hidden.
> > 
> > Long story short, i thought the patch was simple enough to merge even
> > taken out of my special context.
> > 
> > Currently intel_pinctl only works if "ps2b is not hidden by BIOS" or
> > "ACPI tables are correct", lifting the ban on the hidden p2sb seems
> > like a useful thing in general (i.e. sysfs gpio interface). And i was
> > hoping Andy would take the lead on that. It is something my Siemens
> > drivers would depend on, but really a generic thing as far as i
> > understand it.
> 
> From p2sb series discussion it appears that this patch is not needed.
> The case is when BIOS already provides an ACPI device.
> 
> So, the initial bug is in that series that needs to check if the ACPI device is
> exposed and forbid platform device instantiation in that case.

Actually, I'm still thinking how this ever possible. We have all drivers to
provide SoC data pointers. match data may be NULL if and only if the ACPI
device provided is a new one that doesn't provide a SoC data.

So, w/o seeing ACPI table, I'm really puzzled here.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] pinctrl: intel: fix NULL pointer deref

[Henning Schild]

Am Thu, 10 Jun 2021 17:32:46 +0300
schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

> On Thu, Jun 10, 2021 at 05:25:04PM +0300, Andy Shevchenko wrote:
> > On Wed, Jun 09, 2021 at 01:08:16PM +0200, Henning Schild wrote:  
> > > Am Wed, 9 Jun 2021 13:33:34 +0300
> > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:  
> > 
> > ...
> >   
> > > In order to use GPIO from the drivers i need to make sure
> > > "broxton-pinctrl" comes up even if p2sb is hidden.
> > > 
> > > Long story short, i thought the patch was simple enough to merge
> > > even taken out of my special context.
> > > 
> > > Currently intel_pinctl only works if "ps2b is not hidden by BIOS"
> > > or "ACPI tables are correct", lifting the ban on the hidden p2sb
> > > seems like a useful thing in general (i.e. sysfs gpio interface).
> > > And i was hoping Andy would take the lead on that. It is
> > > something my Siemens drivers would depend on, but really a
> > > generic thing as far as i understand it.  
> > 
> > From p2sb series discussion it appears that this patch is not
> > needed. The case is when BIOS already provides an ACPI device.
> > 
> > So, the initial bug is in that series that needs to check if the
> > ACPI device is exposed and forbid platform device instantiation in
> > that case.  
> 
> Actually, I'm still thinking how this ever possible. We have all
> drivers to provide SoC data pointers. match data may be NULL if and
> only if the ACPI device provided is a new one that doesn't provide a
> SoC data.
> 
> So, w/o seeing ACPI table, I'm really puzzled here.

Not sure what exactly you mean. Let us kill this thread and ignore the
patch. It was posted out of context and the NULL deref code-path does
not exist in the kernel, so the check is not needed.

I will revisit the machine where your patch-series did lead to a
double-init and EBUSY on claiming those memory ressources. And i will
add ACPI info there as well.

regards,
Henning

Re: [PATCH] pinctrl: intel: fix NULL pointer deref

[Andy Shevchenko]

On Thu, Jun 10, 2021 at 5:56 PM Henning Schild
<henning.schild@siemens.com> wrote:
>
> Am Thu, 10 Jun 2021 17:32:46 +0300
> schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
>
> > On Thu, Jun 10, 2021 at 05:25:04PM +0300, Andy Shevchenko wrote:
> > > On Wed, Jun 09, 2021 at 01:08:16PM +0200, Henning Schild wrote:
> > > > Am Wed, 9 Jun 2021 13:33:34 +0300
> > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> > >
> > > ...
> > >
> > > > In order to use GPIO from the drivers i need to make sure
> > > > "broxton-pinctrl" comes up even if p2sb is hidden.
> > > >
> > > > Long story short, i thought the patch was simple enough to merge
> > > > even taken out of my special context.
> > > >
> > > > Currently intel_pinctl only works if "ps2b is not hidden by BIOS"
> > > > or "ACPI tables are correct", lifting the ban on the hidden p2sb
> > > > seems like a useful thing in general (i.e. sysfs gpio interface).
> > > > And i was hoping Andy would take the lead on that. It is
> > > > something my Siemens drivers would depend on, but really a
> > > > generic thing as far as i understand it.
> > >
> > > From p2sb series discussion it appears that this patch is not
> > > needed. The case is when BIOS already provides an ACPI device.
> > >
> > > So, the initial bug is in that series that needs to check if the
> > > ACPI device is exposed and forbid platform device instantiation in
> > > that case.
> >
> > Actually, I'm still thinking how this ever possible. We have all
> > drivers to provide SoC data pointers. match data may be NULL if and
> > only if the ACPI device provided is a new one that doesn't provide a
> > SoC data.
> >
> > So, w/o seeing ACPI table, I'm really puzzled here.
>
> Not sure what exactly you mean. Let us kill this thread and ignore the
> patch. It was posted out of context and the NULL deref code-path does
> not exist in the kernel, so the check is not needed.
>
> I will revisit the machine where your patch-series did lead to a
> double-init and EBUSY on claiming those memory ressources. And i will
> add ACPI info there as well.

I guess I got what's going on here. When we create a platform device
we get an associated companion device (which is parent in this case of
LPC) and that's why when we try enumerating it you have got the first
branch chosen.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] pinctrl: intel: fix NULL pointer deref

[Andy Shevchenko]

On Thu, Jun 10, 2021 at 06:00:29PM +0300, Andy Shevchenko wrote:
> On Thu, Jun 10, 2021 at 5:56 PM Henning Schild
> <henning.schild@siemens.com> wrote:
> >
> > Am Thu, 10 Jun 2021 17:32:46 +0300
> > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> >
> > > On Thu, Jun 10, 2021 at 05:25:04PM +0300, Andy Shevchenko wrote:
> > > > On Wed, Jun 09, 2021 at 01:08:16PM +0200, Henning Schild wrote:
> > > > > Am Wed, 9 Jun 2021 13:33:34 +0300
> > > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> > > >
> > > > ...
> > > >
> > > > > In order to use GPIO from the drivers i need to make sure
> > > > > "broxton-pinctrl" comes up even if p2sb is hidden.
> > > > >
> > > > > Long story short, i thought the patch was simple enough to merge
> > > > > even taken out of my special context.
> > > > >
> > > > > Currently intel_pinctl only works if "ps2b is not hidden by BIOS"
> > > > > or "ACPI tables are correct", lifting the ban on the hidden p2sb
> > > > > seems like a useful thing in general (i.e. sysfs gpio interface).
> > > > > And i was hoping Andy would take the lead on that. It is
> > > > > something my Siemens drivers would depend on, but really a
> > > > > generic thing as far as i understand it.
> > > >
> > > > From p2sb series discussion it appears that this patch is not
> > > > needed. The case is when BIOS already provides an ACPI device.
> > > >
> > > > So, the initial bug is in that series that needs to check if the
> > > > ACPI device is exposed and forbid platform device instantiation in
> > > > that case.
> > >
> > > Actually, I'm still thinking how this ever possible. We have all
> > > drivers to provide SoC data pointers. match data may be NULL if and
> > > only if the ACPI device provided is a new one that doesn't provide a
> > > SoC data.
> > >
> > > So, w/o seeing ACPI table, I'm really puzzled here.
> >
> > Not sure what exactly you mean. Let us kill this thread and ignore the
> > patch. It was posted out of context and the NULL deref code-path does
> > not exist in the kernel, so the check is not needed.
> >
> > I will revisit the machine where your patch-series did lead to a
> > double-init and EBUSY on claiming those memory ressources. And i will
> > add ACPI info there as well.
> 
> I guess I got what's going on here. When we create a platform device
> we get an associated companion device (which is parent in this case of
> LPC) and that's why when we try enumerating it you have got the first
> branch chosen.

I have just sent another patch based on this report. Can you please test it?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] pinctrl: intel: fix NULL pointer deref

[Henning Schild]

Am Thu, 10 Jun 2021 18:28:31 +0300
schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

> On Thu, Jun 10, 2021 at 06:00:29PM +0300, Andy Shevchenko wrote:
> > On Thu, Jun 10, 2021 at 5:56 PM Henning Schild
> > <henning.schild@siemens.com> wrote:  
> > >
> > > Am Thu, 10 Jun 2021 17:32:46 +0300
> > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> > >  
> > > > On Thu, Jun 10, 2021 at 05:25:04PM +0300, Andy Shevchenko
> > > > wrote:  
> > > > > On Wed, Jun 09, 2021 at 01:08:16PM +0200, Henning Schild
> > > > > wrote:  
> > > > > > Am Wed, 9 Jun 2021 13:33:34 +0300
> > > > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:  
> > > > >
> > > > > ...
> > > > >  
> > > > > > In order to use GPIO from the drivers i need to make sure
> > > > > > "broxton-pinctrl" comes up even if p2sb is hidden.
> > > > > >
> > > > > > Long story short, i thought the patch was simple enough to
> > > > > > merge even taken out of my special context.
> > > > > >
> > > > > > Currently intel_pinctl only works if "ps2b is not hidden by
> > > > > > BIOS" or "ACPI tables are correct", lifting the ban on the
> > > > > > hidden p2sb seems like a useful thing in general (i.e.
> > > > > > sysfs gpio interface). And i was hoping Andy would take the
> > > > > > lead on that. It is something my Siemens drivers would
> > > > > > depend on, but really a generic thing as far as i
> > > > > > understand it.  
> > > > >
> > > > > From p2sb series discussion it appears that this patch is not
> > > > > needed. The case is when BIOS already provides an ACPI device.
> > > > >
> > > > > So, the initial bug is in that series that needs to check if
> > > > > the ACPI device is exposed and forbid platform device
> > > > > instantiation in that case.  
> > > >
> > > > Actually, I'm still thinking how this ever possible. We have all
> > > > drivers to provide SoC data pointers. match data may be NULL if
> > > > and only if the ACPI device provided is a new one that doesn't
> > > > provide a SoC data.
> > > >
> > > > So, w/o seeing ACPI table, I'm really puzzled here.  
> > >
> > > Not sure what exactly you mean. Let us kill this thread and
> > > ignore the patch. It was posted out of context and the NULL deref
> > > code-path does not exist in the kernel, so the check is not
> > > needed.
> > >
> > > I will revisit the machine where your patch-series did lead to a
> > > double-init and EBUSY on claiming those memory ressources. And i
> > > will add ACPI info there as well.  
> > 
> > I guess I got what's going on here. When we create a platform device
> > we get an associated companion device (which is parent in this case
> > of LPC) and that's why when we try enumerating it you have got the
> > first branch chosen.  
> 
> I have just sent another patch based on this report. Can you please
> test it?

Thanks, that fixed the NULL deref introduced by " [rfc, PATCH v1 0/7]
PCI: introduce p2sb helper", so it should be added to a v2 i guess.

A remaining cosmetic issue is this ...
[    4.131578] broxton-pinctrl apollolake-pinctrl.0: can't request region for resource [mem 0xd0c50000-0xd0c5076b 64bit]
[    4.131669] broxton-pinctrl: probe of apollolake-pinctrl.0 failed with error -16

For all 4 parts. I guess it could detect being already loaded via ACPI
end EBUSY out with INFO instead of ERR.

And i guess if the probing was - for some reason - the other way
around. /sys/class/gpio/gpiochip267/label would be either "INT3452:03"
or "apollolake-pinctrl.3" and a driver building on top would need to
deal with that chip having one of the two names.
I imagine the probing order could change when ACPI gains table entries
with a BIOS update, or looses table entries ...

GPIO_LOOKUP_IDX("apollolake-pinctrl.0" vs. "INT34.."

Same for a userland component using the sysfs GPIO interface and
looking for the chip by "label".

regards,
Henning