[PATCH] gpiolib: Fix crash when exporting non-existant gpio

[PATCH] gpiolib: Fix crash when exporting non-existant gpio

[danielfsantos]

I got this on an RPi and I can't find anything specific to that.
Besides, it's clearly wrong to try to access desc->chip when we have
just tested that it may be NULL at drivers/gpio/gpiolib.c:1409:

	chip = desc->chip;
	if (chip == NULL)
		goto done;

....

done:
	if (status)
		pr_debug("_gpio_request: gpio-%d (%s) status %d\n",
			 desc_to_gpio(desc), label ? : "?", status);

To reproduce, just pick an invalid gpio nubmer and:

echo -n 248 > /sys/class/gpio/export

However, I wasn't able to reproduce it on my laptop, maybe because I
don't have any real gpio chips there, not sure.  More info on RPi bug
report: https://github.com/raspberrypi/linux/issues/364

[  222.961384] Unable to handle kernel NULL pointer dereference at
virtual address 00000044
[  222.969486] pgd = d97d0000
[  222.972190] [00000044] *pgd=1aaca831, *pte=00000000, *ppte=00000000
[  222.978483] Internal error: Oops: 17 [#1] PREEMPT ARM
---
 drivers/gpio/gpiolib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index d6413b2..e547f75f8b 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -136,7 +136,7 @@ static struct gpio_desc *gpio_to_desc(unsigned gpio)
  */
 static int desc_to_gpio(const struct gpio_desc *desc)
 {
-	return desc->chip->base + gpio_chip_hwgpio(desc);
+	return desc->chip ?  desc->chip->base + gpio_chip_hwgpio(desc) : -1;
 }
 
 
-- 
1.8.1.5

Re: [PATCH] gpiolib: Fix crash when exporting non-existant gpio

[Guenter Roeck]

On 08/24/2013 11:48 AM, danielfsantos@att.net wrote:
> I got this on an RPi and I can't find anything specific to that.
> Besides, it's clearly wrong to try to access desc->chip when we have
> just tested that it may be NULL at drivers/gpio/gpiolib.c:1409:
>
> 	chip = desc->chip;
> 	if (chip == NULL)
> 		goto done;
>
> ....
>
> done:
> 	if (status)
> 		pr_debug("_gpio_request: gpio-%d (%s) status %d\n",
> 			 desc_to_gpio(desc), label ? : "?", status);
>
> To reproduce, just pick an invalid gpio nubmer and:
>
> echo -n 248 > /sys/class/gpio/export
>
> However, I wasn't able to reproduce it on my laptop, maybe because I
> don't have any real gpio chips there, not sure.  More info on RPi bug
> report: https://github.com/raspberrypi/linux/issues/364
>
> [  222.961384] Unable to handle kernel NULL pointer dereference at
> virtual address 00000044
> [  222.969486] pgd = d97d0000
> [  222.972190] [00000044] *pgd=1aaca831, *pte=00000000, *ppte=00000000
> [  222.978483] Internal error: Oops: 17 [#1] PREEMPT ARM
> ---
>   drivers/gpio/gpiolib.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index d6413b2..e547f75f8b 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -136,7 +136,7 @@ static struct gpio_desc *gpio_to_desc(unsigned gpio)
>    */
>   static int desc_to_gpio(const struct gpio_desc *desc)
>   {
> -	return desc->chip->base + gpio_chip_hwgpio(desc);
> +	return desc->chip ?  desc->chip->base + gpio_chip_hwgpio(desc) : -1;
>   }
>

Looking into calling code, desc_to_gpio() is clearly not supposed to return an error,
and it will result in odd behavior if it returns -1. For example, the resulting debug
message of "gpio--1 (...) status ..." is not very useful.

It would make more sense to fix the calling code. You could for example
validate in affected functions if the gpio pin exists by not only
checking for desc but also for desc->chip. Another option might be
to have gpio_to_desc() return NULL if desc->chip is NULL.

Guenter

Re: [PATCH] gpiolib: Fix crash when exporting non-existant gpio

[Daniel Santos]

On 08/24/2013 02:57 PM, Guenter Roeck wrote:
>
> Looking into calling code, desc_to_gpio() is clearly not supposed to 
> return an error,
> and it will result in odd behavior if it returns -1. For example, the 
> resulting debug
> message of "gpio--1 (...) status ..." is not very useful.
>
> It would make more sense to fix the calling code. You could for example
> validate in affected functions if the gpio pin exists by not only
> checking for desc but also for desc->chip. Another option might be
> to have gpio_to_desc() return NULL if desc->chip is NULL.

Yes, you are correct of course.  I guess I was just being lazy. :) I'll 
re-submit.

[PATCH] gpiolib: Fix crash when exporting non-existant gpio

[danielfsantos]

I got this on an RPi and I can't find anything specific to that.
Besides, it's clearly wrong to try to access desc->chip when we have
just tested that it may be NULL at drivers/gpio/gpiolib.c:1409:

	chip = desc->chip;
	if (chip == NULL)
		goto done;

....

done:
	if (status)
		pr_debug("_gpio_request: gpio-%d (%s) status %d\n",
			 desc_to_gpio(desc), label ? : "?", status);

To reproduce, just pick an invalid gpio nubmer and:

echo -n 248 > /sys/class/gpio/export

However, I wasn't able to reproduce it on my laptop, maybe because I
don't have any real gpio chips there, not sure.  More info on RPi bug
report: https://github.com/raspberrypi/linux/issues/364

This fix makes sure that gpio_to_desc() only returns non-NULL if the
specified gpio really has a chip, and not just if it's within the ranged
of gpios for the arch.

[  222.961384] Unable to handle kernel NULL pointer dereference at
virtual address 00000044
[  222.969486] pgd = d97d0000
[  222.972190] [00000044] *pgd=1aaca831, *pte=00000000, *ppte=00000000
[  222.978483] Internal error: Oops: 17 [#1] PREEMPT ARM
---
 drivers/gpio/gpiolib.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index d6413b2..db7c6bb 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -123,7 +123,8 @@ static int gpio_chip_hwgpio(const struct gpio_desc *desc)
  */
 static struct gpio_desc *gpio_to_desc(unsigned gpio)
 {
-	if (WARN(!gpio_is_valid(gpio), "invalid GPIO %d\n", gpio))
+	if (WARN(!gpio_is_valid(gpio) || !gpio_desc[gpio].chip,
+			"invalid GPIO %d\n", gpio))
 		return NULL;
 	else
 		return &gpio_desc[gpio];
@@ -1406,8 +1407,7 @@ static int gpiod_request(struct gpio_desc *desc, const char *label)
 	spin_lock_irqsave(&gpio_lock, flags);
 
 	chip = desc->chip;
-	if (chip == NULL)
-		goto done;
+	BUG_ON(!chip);
 
 	if (!try_module_get(chip->owner))
 		goto done;
-- 
1.8.1.5

Re: [PATCH v2] gpiolib: Fix crash when exporting non-existent gpio

[Daniel Santos]

hmm, git send-email didn't change my subject on the above message to 
"[PATCH v2]", not sure what I did wrong.  Sorry about that.

Daniel

Re: [PATCH] gpiolib: Fix crash when exporting non-existant gpio

[Guenter Roeck]

On 08/24/2013 01:48 PM, danielfsantos@att.net wrote:
> I got this on an RPi and I can't find anything specific to that.
> Besides, it's clearly wrong to try to access desc->chip when we have
> just tested that it may be NULL at drivers/gpio/gpiolib.c:1409:
>
> 	chip = desc->chip;
> 	if (chip == NULL)
> 		goto done;
>
> ....
>
> done:
> 	if (status)
> 		pr_debug("_gpio_request: gpio-%d (%s) status %d\n",
> 			 desc_to_gpio(desc), label ? : "?", status);
>
> To reproduce, just pick an invalid gpio nubmer and:
>
> echo -n 248 > /sys/class/gpio/export
>
> However, I wasn't able to reproduce it on my laptop, maybe because I
> don't have any real gpio chips there, not sure.  More info on RPi bug
> report: https://github.com/raspberrypi/linux/issues/364
>
> This fix makes sure that gpio_to_desc() only returns non-NULL if the
> specified gpio really has a chip, and not just if it's within the ranged
> of gpios for the arch.
>
> [  222.961384] Unable to handle kernel NULL pointer dereference at
> virtual address 00000044
> [  222.969486] pgd = d97d0000
> [  222.972190] [00000044] *pgd=1aaca831, *pte=00000000, *ppte=00000000
> [  222.978483] Internal error: Oops: 17 [#1] PREEMPT ARM
> ---
>   drivers/gpio/gpiolib.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index d6413b2..db7c6bb 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -123,7 +123,8 @@ static int gpio_chip_hwgpio(const struct gpio_desc *desc)
>    */
>   static struct gpio_desc *gpio_to_desc(unsigned gpio)
>   {
> -	if (WARN(!gpio_is_valid(gpio), "invalid GPIO %d\n", gpio))
> +	if (WARN(!gpio_is_valid(gpio) || !gpio_desc[gpio].chip,
> +			"invalid GPIO %d\n", gpio))

I think this triggers a WARN if someone requests an invalid gpio pin from userspace.
Is this really a good idea ?

[ and then export_store and unexport_store complain again with pr_warn ]

May be a separate patch, but if the WARN is useful it might make sense to introduce
gpio_to_desc_silent() which doesn't produce the WARN if it fails.

Looking further into the code, I suspect there may be some race condition
where desc->chip is not (yet) set and export_store is called. So we will
see a WARNING instead of a crash, as the underlying condition still exists.

>   		return NULL;
>   	else
>   		return &gpio_desc[gpio];
> @@ -1406,8 +1407,7 @@ static int gpiod_request(struct gpio_desc *desc, const char *label)
>   	spin_lock_irqsave(&gpio_lock, flags);
>
>   	chip = desc->chip;
> -	if (chip == NULL)
> -		goto done;
> +	BUG_ON(!chip);
>
... which in turn means we might see this one. If so, this code might replace
an invalid memory access crash with a BUG crash. Is this really desirable, or
should this better be a WARN ?

Guenter


>   	if (!try_module_get(chip->owner))
>   		goto done;
>

Re: [PATCH] gpiolib: Fix crash when exporting non-existant gpio

[Linus Walleij]

On Sat, Aug 24, 2013 at 10:48 PM,  <danielfsantos@att.net> wrote:

> [  222.961384] Unable to handle kernel NULL pointer dereference at
> virtual address 00000044
> [  222.969486] pgd = d97d0000
> [  222.972190] [00000044] *pgd=1aaca831, *pte=00000000, *ppte=00000000
> [  222.978483] Internal error: Oops: 17 [#1] PREEMPT ARM
> ---
>  drivers/gpio/gpiolib.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index d6413b2..db7c6bb 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -123,7 +123,8 @@ static int gpio_chip_hwgpio(const struct gpio_desc *desc)
>   */
>  static struct gpio_desc *gpio_to_desc(unsigned gpio)
>  {
> -       if (WARN(!gpio_is_valid(gpio), "invalid GPIO %d\n", gpio))
> +       if (WARN(!gpio_is_valid(gpio) || !gpio_desc[gpio].chip,
> +                       "invalid GPIO %d\n", gpio))
>                 return NULL;
>         else
>                 return &gpio_desc[gpio];
> @@ -1406,8 +1407,7 @@ static int gpiod_request(struct gpio_desc *desc, const char *label)
>         spin_lock_irqsave(&gpio_lock, flags);
>
>         chip = desc->chip;
> -       if (chip == NULL)
> -               goto done;
> +       BUG_ON(!chip);

It'd be good if Alexandre took a look at this.

BUG_ON() is pretty nasty, atleast replace it with
a warning.

Yours,
Linus Walleij

Re: [PATCH] gpiolib: Fix crash when exporting non-existant gpio

[Alexandre Courbot]

On Thu, Aug 29, 2013 at 6:52 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Sat, Aug 24, 2013 at 10:48 PM,  <danielfsantos@att.net> wrote:
>
>> [  222.961384] Unable to handle kernel NULL pointer dereference at
>> virtual address 00000044
>> [  222.969486] pgd = d97d0000
>> [  222.972190] [00000044] *pgd=1aaca831, *pte=00000000, *ppte=00000000
>> [  222.978483] Internal error: Oops: 17 [#1] PREEMPT ARM
>> ---
>>  drivers/gpio/gpiolib.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> index d6413b2..db7c6bb 100644
>> --- a/drivers/gpio/gpiolib.c
>> +++ b/drivers/gpio/gpiolib.c
>> @@ -123,7 +123,8 @@ static int gpio_chip_hwgpio(const struct gpio_desc *desc)
>>   */
>>  static struct gpio_desc *gpio_to_desc(unsigned gpio)
>>  {
>> -       if (WARN(!gpio_is_valid(gpio), "invalid GPIO %d\n", gpio))
>> +       if (WARN(!gpio_is_valid(gpio) || !gpio_desc[gpio].chip,
>> +                       "invalid GPIO %d\n", gpio))
>>                 return NULL;
>>         else
>>                 return &gpio_desc[gpio];
>> @@ -1406,8 +1407,7 @@ static int gpiod_request(struct gpio_desc *desc, const char *label)
>>         spin_lock_irqsave(&gpio_lock, flags);
>>
>>         chip = desc->chip;
>> -       if (chip == NULL)
>> -               goto done;
>> +       BUG_ON(!chip);
>
> It'd be good if Alexandre took a look at this.
>
> BUG_ON() is pretty nasty, atleast replace it with
> a warning.

Agreed - that's a cheap way to crash the kernel. desc_to_gpio()
assumes a valid descriptor, so we should not call it from contexts
where we know the descriptor may be invalid. How about having the
initial "if (!desc)" changed into "if (!desc || !desc->chip)" instead?
That way an error would be returned immediatly and we would know we
have a valid descriptor after that.

Having gpio_to_desc() return NULL if the descriptor does not have a
chip associated also seems to make sense - otherwise the caller should
check it itself. I'd even go as far as saying the check should be done
in gpio_is_valid itself.

There is probably a lot more potential to improve error handling in
gpiolib. Generally speaking, moving safety checks to a lower-level and
propagating error codes accordingly should be the right approach, as
long as it doesn't clutter performance. We want to be able to assume
that GPIO descriptors are valid in most of the code.

Alex.