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@xxxxxxxxxx> wrote:
> On Sat, Aug 24, 2013 at 10:48 PM,  <danielfsantos@xxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/