Received-SPF: pass (google.com: domain of linux-kernel+bounces-171241-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <730734c0-59a6-4612-90ee-9715ab832515@quicinc.com>
Date: Tue, 7 May 2024 20:09:14 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] gpiolib: cdev: Fix use after free in
 lineinfo_changed_notify
To: Bartosz Golaszewski <brgl@bgdev.pl>
CC: <warthog618@gmail.com>, <linus.walleij@linaro.org>,
        <linux-gpio@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20240505141156.2944912-1-quic_zhonhan@quicinc.com>
 <CAMRc=MdmOg6pJ7hvKSpkoTKjQny8xL5BFT2HNzgKgnjsCuwhwQ@mail.gmail.com>
Content-Language: en-US
From: Zhongqiu Han <quic_zhonhan@quicinc.com>
In-Reply-To: <CAMRc=MdmOg6pJ7hvKSpkoTKjQny8xL5BFT2HNzgKgnjsCuwhwQ@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit

On 5/7/2024 4:00 PM, Bartosz Golaszewski wrote:
> On Sun, May 5, 2024 at 4:12 PM Zhongqiu Han <quic_zhonhan@quicinc.com> wrote:
>>
>> The use-after-free issue occurs as follows: when the GPIO chip device file
>> is being closed by invoking gpio_chrdev_release(), watched_lines is freed
>> by bitmap_free(), but the unregistration of lineinfo_changed_nb notifier
>> chain failed due to waiting write rwsem. Additionally, one of the GPIO
>> chip's lines is also in the release process and holds the notifier chain's
>> read rwsem. Consequently, a race condition leads to the use-after-free of
>> watched_lines.
>>
>> Here is the typical stack when issue happened:
>>
>> [free]
>> gpio_chrdev_release()
>>    --> bitmap_free(cdev->watched_lines)                  <-- freed
>>    --> blocking_notifier_chain_unregister()
>>      --> down_write(&nh->rwsem)                          <-- waiting rwsem
>>            --> __down_write_common()
>>              --> rwsem_down_write_slowpath()
>>                    --> schedule_preempt_disabled()
>>                      --> schedule()
>>
> 
> The rwsem has been removed in v6.9-rc1. I assume you're targeting
> stable branches with this change? Or does it still occur with the
> recent SRCU rework? This is important to know before I send it
> upstream.
> 
> Bart
> 

Hi Bart,

Thanks a lot for the review.

I guess the "rwsem -> srcu switch" you mentioned is this commit
d83cee3d2bb1 ("gpio: protect the pointer to gpio_chip in gpio_device
with SRCU")? Currently the use-after-free issue of watched_lines is
related to notifier chain rwsem instead of the rwsem which is as one
struct member of gpio_device.

int blocking_notifier_chain_unregister(struct blocking_notifier_head *nh,
         struct notifier_block *n)
{
     int ret;

     /*
      * This code gets used during boot-up, when task switching is
      * not yet working and interrupts must remain disabled.  At
      * such times we must not call down_write().
      */
     if (unlikely(system_state == SYSTEM_BOOTING))
         return notifier_chain_unregister(&nh->head, n);

     down_write(&nh->rwsem);------------------->waiting rwsem here
     ret = notifier_chain_unregister(&nh->head, n);
     up_write(&nh->rwsem);
     return ret;
}

struct blocking_notifier_head {
     struct rw_semaphore rwsem;
     struct notifier_block __rcu *head;
};

Please forgive me for not explaining rwsem on the commit message and
kindly let me know if there is any misunderstanding. Thank you~

>> [use]
>> st54spi_gpio_dev_release()
>>    --> gpio_free()
>>      --> gpiod_free()
>>        --> gpiod_free_commit()
>>          --> gpiod_line_state_notify()
>>            --> blocking_notifier_call_chain()
>>              --> down_read(&nh->rwsem);                  <-- held rwsem
>>              --> notifier_call_chain()
>>                --> lineinfo_changed_notify()
>>                  --> test_bit(xxxx, cdev->watched_lines) <-- use after free
>>
>> The side effect of the use-after-free issue is that a GPIO line event is
>> being generated for userspace where it shouldn't. However, since the chrdev
>> is being closed, userspace won't have the chance to read that event anyway.
>>
>> To fix the issue, call the bitmap_free() function after the unregistration
>> of lineinfo_changed_nb notifier chain.
>>
>> Fixes: 51c1064e82e7 ("gpiolib: add new ioctl() for monitoring changes in line info")
>> Signed-off-by: Zhongqiu Han <quic_zhonhan@quicinc.com>
>> ---
>> v1 -> v2:
>> - Drop the excessive stack log from commit message to make it more readable.
>> - Add a note regarding the side effects of the use-after-free on commit message.
>> - Link to v1: https://lore.kernel.org/lkml/20240501022612.1787143-1-quic_zhonhan@quicinc.com/
>>
>>   drivers/gpio/gpiolib-cdev.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
>> index d09c7d728365..6b3a43e3ba47 100644
>> --- a/drivers/gpio/gpiolib-cdev.c
>> +++ b/drivers/gpio/gpiolib-cdev.c
>> @@ -2799,11 +2799,11 @@ static int gpio_chrdev_release(struct inode *inode, struct file *file)
>>          struct gpio_chardev_data *cdev = file->private_data;
>>          struct gpio_device *gdev = cdev->gdev;
>>
>> -       bitmap_free(cdev->watched_lines);
>>          blocking_notifier_chain_unregister(&gdev->device_notifier,
>>                                             &cdev->device_unregistered_nb);
>>          blocking_notifier_chain_unregister(&gdev->line_state_notifier,
>>                                             &cdev->lineinfo_changed_nb);
>> +       bitmap_free(cdev->watched_lines);
>>          gpio_device_put(gdev);
>>          kfree(cdev);
>>
>> --
>> 2.25.1
>>

-- 
Thx and BRs,
Zhongqiu Han