On Wed, 23 Aug, 2023 07:55:17 -0700 Dmitry Torokhov [off-list ref] wrote:

Hi Maxime,

On Wed, Aug 23, 2023 at 03:16:02PM +0200, Maxime Ripard wrote:

quoted

Hi Dmitry,

On Wed, Aug 23, 2023 at 05:51:00AM -0700, Dmitry Torokhov wrote:

quoted

On Wed, Aug 23, 2023 at 09:44:22AM +0200, Maxime Ripard wrote:

quoted

On Tue, Aug 22, 2023 at 08:57:41AM -0700, Rahul Rameshbabu wrote:

quoted

On Tue, 22 Aug, 2023 11:12:28 +0200 Maxime Ripard [off-list ref] wrote:

quoted

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).

I'm sorry, I don't know the input framework well enough to understand
what you had in mind exactly. Could you send a patch with your
suggestion for the hid-nvidia-shield so we can discuss this further?

That being said, I think that the current design around name, phys and
uniq is fairly treacherous to drivers and we should aim for a solution
that prevents that issue from being possible at all.

I was inclined to go for a char array for each to get rid of the pointer
entirely, but Benjamin raised some concerns over the structure size so
it's probably not a great solution.

I think everything is much simpler, with uclogic driver being in the
wrong here: devm resource needs to be attached to the right device
(instance of HID) rather than to the input device itself (which should
never have any driver resources attached since it never has a driver).
Something like this:

diff --git a/drivers/hid/hid-uclogic-core.c b/drivers/hid/hid-uclogic-core.c
index f67835f9ed4c..f234a7c97360 100644
--- a/drivers/hid/hid-uclogic-core.c
+++ b/drivers/hid/hid-uclogic-core.c

@@ -148,7 +148,7 @@ static int uclogic_input_configured(struct hid_device *hdev,
 
 	if (suffix) {
 		len = strlen(hdev->name) + 2 + strlen(suffix);
-		name = devm_kzalloc(&hi->input->dev, len, GFP_KERNEL);
+		name = devm_kzalloc(&hdev->dev, len, GFP_KERNEL);
 		if (name) {
 			snprintf(name, len, "%s %s", hdev->name, suffix);
 			hi->input->name = name;

In general, drivers should attach devm resources they allocate to the
instance of device they are binding to, and nothing else.

I'm not sure that's enough unfortunately. The fundamental issue here
seems to be that input_dev_uevent follows a pointer that can be
allocated by the driver, and will be free'd before the last call to
input_dev_uevent.

Yes, this is a fundamental property of C pointers - you should not free
them before exiting last code section that may reference them. For input
devices it means that pointers should not be freed until after
input_unregister_device() is called.

I.e. you have sequence like this:

	driver_data = kzalloc(...);
	driver_data->input_name = kstrdup(...);
	driver_data->input_phys = kstrdup(...);
	input = input_allocate_device();
	input->name = driver_data->input_name;
	input->phys = driver_data->input_phys;
	input_register_device(input);
	...

	input_unregister_device(input);
	kfree(driver_data->input_name);
	kfree(driver_data->input_phys);
	kfree(driver_data);


devm typically helps with resources being freed at the right time, but
for that the managed resources should be attached to the *correct
device*, with correct device being one the driver is binding to, not any
random device structure.

quoted

And I think that's true for both devices here

Yes, it looks like the shield is also using wrong device.

This is a problem in shield too. I'll submit a patch. I'll take a look
at other drivers as well to see if any of them run into this issue.

	idev->name = devm_kasprintf(&idev->dev, GFP_KERNEL, "%s %s", hdev->name,
				    name_suffix);

--
Thanks,

Rahul Rameshbabu