Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH] gpio: Restrict usage of gc irq members before
 initialization
To:     Andy Shevchenko <andy.shevchenko@gmail.com>
Cc:     Linus Walleij <linus.walleij@linaro.org>,
        Bartosz Golaszewski <brgl@bgdev.pl>, krisman@collabora.com,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Collabora Kernel ML <kernel@collabora.com>
References: <20220310132108.225387-1-shreeya.patel@collabora.com>
 <CAHp75VdGWxnxRzEhSknXHRZf1SX0x=pvca_CL0VinWNFcqCQwg@mail.gmail.com>
From:   Shreeya Patel <shreeya.patel@collabora.com>
Message-ID: <c08664da-d83f-8789-cc8c-5ae3339d9b89@collabora.com>
Date:   Fri, 11 Mar 2022 19:29:59 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <CAHp75VdGWxnxRzEhSknXHRZf1SX0x=pvca_CL0VinWNFcqCQwg@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Precedence: bulk


On 10/03/22 8:14 pm, Andy Shevchenko wrote:
> On Thu, Mar 10, 2022 at 3:22 PM Shreeya Patel
> <shreeya.patel@collabora.com> wrote:
>> gc irq members are exposed before they could be completely
>> initialized and this leads to race conditions.
>>
>> One such issue was observed for the gc->irq.domain variable which
>> was accessed through the I2C interface in gpiochip_to_irq() before
>> it could be initialized by gpiochip_add_irqchip(). This resulted in
>> Kernel NULL pointer dereference.
>>
>> To avoid such scenarios, restrict usage of gc irq members before
>> they are completely initialized.
> Fixes tag?
>
> ...


We don't really have any specific commit which introduces this issue so
I did not add fixes tag here.

>> +bool gc_irq_initialized;
> Non-static?
>
> Why is it global?
>
> ...
>
>> @@ -1593,6 +1594,8 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
>>
>>          acpi_gpiochip_request_interrupts(gc);
>>
>> +       gc_irq_initialized = true;
> This is wrong. Imagine a system where you have more than one GPIO chip.
>
> ...


Yes, thanks for pointing it out. This flag introduced should be a member of
gpio_chip structure as mentioned by Gabriel and then it should work for
multiple gpio chips as well.

>> -       if (gc->to_irq) {
>> +       if (gc->to_irq && gc_irq_initialized) {
>>                  int retirq = gc->to_irq(gc, offset);
> Shouldn't it rather be something like
>
>    if (gc->to_irq) {
>      if (! ..._initialized)
>        return -EPROBE_DEFER;
>      ...
>    }
>
> ?


No, the above code will still bring in issues when gc->to_irq is NULL
and will return -ENXIO. We have seen race in registering of gc->to_irq
as well which is why we had introduced the following patch as the first 
step.

https://lore.kernel.org/lkml/20220216202655.194795-1-shreeya.patel@collabora.com/t/


Thanks,
Shreeya Patel

>
> --
> With Best Regards,
> Andy Shevchenko