Message-ID: <874jkqn1u4.fsf@nvidia.com>
Date:   Tue, 22 Aug 2023 15:34:27 -0700
From:   Rahul Rameshbabu <rrameshbabu@...dia.com>
To:     Maxime Ripard <mripard@...nel.org>
Cc:     syzbot <syzbot+3a0ebe8a52b89c63739d@...kaller.appspotmail.com>,
        davidgow@...gle.com, dmitry.torokhov@...il.com,
        gregkh@...uxfoundation.org, linux-input@...r.kernel.org,
        linux-kernel@...r.kernel.org, rydberg@...math.org,
        syzkaller-bugs@...glegroups.com, benjamin.tissoires@...hat.com
Subject: Re: [syzbot] [input?] KASAN: slab-use-after-free Read in
 input_dev_uevent

On Tue, 22 Aug, 2023 08:57:41 -0700 Rahul Rameshbabu <rrameshbabu@...dia.com> wrote:
> Hi Maxime,
>
> On Tue, 22 Aug, 2023 11:12:28 +0200 Maxime Ripard <mripard@...nel.org> wrote:
>> Hi,
>>
>> So, we discussed it this morning with Benjamin, and I think the culprit
>> is that the uclogic driver will allocate a char array with devm_kzalloc
>> in uclogic_input_configured()
>> (https://elixir.bootlin.com/linux/latest/source/drivers/hid/hid-uclogic-core.c#L149),
>> and will assign input_dev->name to that pointer.
>>
>> When the device is removed, the devm-allocated array is freed, and the
>> input framework will send a uevent in input_dev_uevent() using the
>> input_dev->name field:
>>
>> https://elixir.bootlin.com/linux/latest/source/drivers/input/input.c#L1688
>>
>> So it's a classic dangling pointer situation.
>>
>> And even though it was revealed by that patch, I think the issue is
>> unrelated. The fundamental issue seems to be that the usage of devm in
>> that situation is wrong.
>>
>> input_dev->name is accessed by input_dev_uevent, which for KOBJ_UNBIND
>> and KOBJ_REMOVE will be called after remove.
>>
>> For example, in __device_release_driver() (with the driver remove hook
>> being called in device_remove() and devres_release_all() being called in
>> device_unbind_cleanup()):
>> https://elixir.bootlin.com/linux/latest/source/drivers/base/dd.c#L1278
>>
>> So, it looks to me that, with or without the patch we merged recently,
>> the core has always sent uevent after device-managed resources were
>> freed. Thus, the uclogic (and any other input driver) was wrong in
>> allocating its input_dev name with devm_kzalloc (or the phys and uniq
>> fields in that struct).
>>
>> Note that freeing input_dev->name in remove would have been just as bad.
>>
>> Looking at the code quickly, at least hid-playstation,
>> hid-nvidia-shield, hid-logitech-hidpp, mms114 and tsc200x seem to be
>> affected by the same issue.
>
> I agree with this analysis overall. At least in hid-nvidia-shield, I can
> not use devm for allocating the input name string and explicitly free it
> after calling input_unregister_device. In this scenario, the name string
> would have been freed explicitly after input_put_device was called
> (since the input device is not devres managed). input_put_device would
> drop the reference count to zero and the device would be cleaned up at
> that point triggering KOBJ_REMOVE and firing off that final
> input_dev_uevent.
>
> I think this can be done for a number of the drivers as a workaround
> till this issue is properly resolved. If this seems appropriate, I can
> send out a series later in the day. This is just a workaround till the
> discussion below converges (which I am interested in).

After thinking about it a bit further, I believe hid-nvidia-shield is
not impacted by this issue in its current state. hid_hw_stop will
trigger devres_release_all, which will free the input name allocated.
This is problematic in the case that the input_dev is devres managed,
since device resource management will hook the uevent callback on
device_del triggered by input_unregister_device invoked by hid_hw_stop.
However, hid-nvidia-shield does not use devm to allocate the input
device and explicitly calls input_unregister_device before the call to
hid_hw_stop. I believe this leads to the uevent being fired before name
is freed by devres after the hid_hw_stop call. Let me know if this
analysis seems off. Hoping this can be helpful in general if this is the
case.

  Freed by task 4508:
   kasan_save_stack+0x33/0x50 mm/kasan/common.c:45
   kasan_set_track+0x25/0x30 mm/kasan/common.c:52
   kasan_save_free_info+0x2b/0x40 mm/kasan/generic.c:522
   ____kasan_slab_free mm/kasan/common.c:236 [inline]
   ____kasan_slab_free+0x15b/0x1b0 mm/kasan/common.c:200
   kasan_slab_free include/linux/kasan.h:164 [inline]
   slab_free_hook mm/slub.c:1800 [inline]
   slab_free_freelist_hook+0x114/0x1e0 mm/slub.c:1826
   slab_free mm/slub.c:3809 [inline]
   __kmem_cache_free+0xb8/0x2f0 mm/slub.c:3822
   release_nodes drivers/base/devres.c:506 [inline]
   devres_release_all+0x192/0x240 drivers/base/devres.c:535
   device_del+0x628/0xa50 drivers/base/core.c:3827
   input_unregister_device+0xb9/0x100 drivers/input/input.c:2440
   hidinput_disconnect+0x160/0x3e0 drivers/hid/hid-input.c:2386
   hid_disconnect+0x143/0x1b0 drivers/hid/hid-core.c:2273
   hid_hw_stop+0x16/0x70 drivers/hid/hid-core.c:2322
   uclogic_remove+0x47/0x90 drivers/hid/hid-uclogic-core.c:485

>
>>
>> We discussed a couple of solutions with Benjamin, such as creating a
>> helper devm action to free and clear the input_dev->name field, droping
>> the name, phys and uniq fields from the uevent, or converting name, phys
>> and uniq to char arrays so drivers don't have to allocate them.
>>
>> We couldn't find a perfect one though, so... yeah.
>>
>> Maxime
>
--
Thanks,

Rahul Rameshbabu

Powered by blists - more mailing lists