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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Thu, 29 Aug 2013 19:23:55 +0900
From:	Alexandre Courbot <gnurou@...il.com>
To:	Linus Walleij <linus.walleij@...aro.org>
Cc:	Daniel Santos <daniel.santos@...ox.com>,
	Alexandre Courbot <acourbot@...dia.com>,
	"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Guenter Roeck <linux@...ck-us.net>
Subject: Re: [PATCH] gpiolib: Fix crash when exporting non-existant gpio

On Thu, Aug 29, 2013 at 6:52 PM, Linus Walleij <linus.walleij@...aro.org> wrote:
> On Sat, Aug 24, 2013 at 10:48 PM,  <danielfsantos@....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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/