[PATCH HID 0/3] Fix devm references used in HID drivers when allocating input_dev name

[PATCH HID 0/3] Fix devm references used in HID drivers when allocating input_dev name

[Rahul Rameshbabu]

Maxime Ripard analyzed the following situation involving a use-after-free caused
by incorrect devres management.

  1. input_dev name allocated as a resource referring to the same input_dev
     instance
  2. The input_dev is eventually unregistered
  3. Unregistering the device first involves releasing devres managed resources
     tied to the input_dev
  4. A uevent is then fired for the input_dev, referencing various members of
     the input_dev including the name
  5. This leads to a use-after-free in the context of the triggered uevent

Dmitry Torokhov pointed out that the correct pattern for devm usage with the
input_dev would be to allocate the resource referencing the underlying device
that was probed by the driver than referencing the input subdevice instance. In
the case of hid drivers, the name resource will only be freed when devres
management reclaims resources for the hid_device. This will be after the
input_dev was unregistered and the uevent referencing the name was invoked.

This patch series applies the analysis done to correct problematic HID drivers.

Link: https://lore.kernel.org/linux-input/ZOZIZCND+L0P1wJc@penguin/T/#m443f3dce92520f74b6cf6ffa8653f9c92643d4ae

Rahul Rameshbabu (3):
  HID: uclogic: Correct devm device reference for hidinput input_dev
    name
  HID: multitouch: Correct devm device reference for hidinput input_dev
    name
  HID: nvidia-shield: Reference hid_device devm allocation of input_dev
    name

 drivers/hid/hid-multitouch.c    | 13 +++----------
 drivers/hid/hid-nvidia-shield.c |  2 +-
 drivers/hid/hid-uclogic-core.c  | 13 +++----------
 3 files changed, 7 insertions(+), 21 deletions(-)

-- 
2.40.1

[PATCH HID 1/3] HID: uclogic: Correct devm device reference for hidinput input_dev name

[Rahul Rameshbabu]

Reference the HID device rather than the input device for the devm
allocation of the input_dev name. Referencing the input_dev would lead to a
use-after-free when the input_dev was unregistered and subsequently fires a
uevent that depends on the name. At the point of firing the uevent, the
name would be freed by devres management.

Use devm_kasprintf to simplify the logic for allocating memory and
formatting the input_dev name string.

Reported-by: syzbot+3a0ebe8a52b89c63739d@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/linux-input/ZOZIZCND+L0P1wJc@penguin/T/
Reported-by: Maxime Ripard <mripard@kernel.org>
Closes: https://lore.kernel.org/linux-input/ZOZIZCND+L0P1wJc@penguin/T/#m443f3dce92520f74b6cf6ffa8653f9c92643d4ae
Fixes: cce2dbdf258e ("HID: uclogic: name the input nodes based on their tool")
Suggested-by: Maxime Ripard <mripard@kernel.org>
Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Rahul Rameshbabu <sergeantsagara@protonmail.com>
---
 drivers/hid/hid-uclogic-core.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/hid/hid-uclogic-core.c b/drivers/hid/hid-uclogic-core.c
index f67835f9ed4c..ad74cbc9a0aa 100644
--- a/drivers/hid/hid-uclogic-core.c
+++ b/drivers/hid/hid-uclogic-core.c
@@ -85,10 +85,8 @@ static int uclogic_input_configured(struct hid_device *hdev,
 {
 	struct uclogic_drvdata *drvdata = hid_get_drvdata(hdev);
 	struct uclogic_params *params = &drvdata->params;
-	char *name;
 	const char *suffix = NULL;
 	struct hid_field *field;
-	size_t len;
 	size_t i;
 	const struct uclogic_params_frame *frame;
 
@@ -146,14 +144,9 @@ 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);
-		if (name) {
-			snprintf(name, len, "%s %s", hdev->name, suffix);
-			hi->input->name = name;
-		}
-	}
+	if (suffix)
+		hi->input->name = devm_kasprintf(&hdev->dev, GFP_KERNEL,
+						 "%s %s", hdev->name, suffix);
 
 	return 0;
 }
-- 
2.40.1

[PATCH HID 2/3] HID: multitouch: Correct devm device reference for hidinput input_dev name

[Rahul Rameshbabu]

Reference the HID device rather than the input device for the devm
allocation of the input_dev name. Referencing the input_dev would lead to a
use-after-free when the input_dev was unregistered and subsequently fires a
uevent that depends on the name. At the point of firing the uevent, the
name would be freed by devres management.

Use devm_kasprintf to simplify the logic for allocating memory and
formatting the input_dev name string.

Reported-by: Maxime Ripard <mripard@kernel.org>
Closes: https://lore.kernel.org/linux-input/ZOZIZCND+L0P1wJc@penguin/T/#m443f3dce92520f74b6cf6ffa8653f9c92643d4ae
Fixes: c08d46aa805b ("HID: multitouch: devm conversion")
Suggested-by: Maxime Ripard <mripard@kernel.org>
Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Rahul Rameshbabu <sergeantsagara@protonmail.com>
---
 drivers/hid/hid-multitouch.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index e31be0cb8b85..521b2ffb4244 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -1594,7 +1594,6 @@ static void mt_post_parse(struct mt_device *td, struct mt_application *app)
 static int mt_input_configured(struct hid_device *hdev, struct hid_input *hi)
 {
 	struct mt_device *td = hid_get_drvdata(hdev);
-	char *name;
 	const char *suffix = NULL;
 	struct mt_report_data *rdata;
 	struct mt_application *mt_application = NULL;
@@ -1645,15 +1644,9 @@ static int mt_input_configured(struct hid_device *hdev, struct hid_input *hi)
 		break;
 	}
 
-	if (suffix) {
-		name = devm_kzalloc(&hi->input->dev,
-				    strlen(hdev->name) + strlen(suffix) + 2,
-				    GFP_KERNEL);
-		if (name) {
-			sprintf(name, "%s %s", hdev->name, suffix);
-			hi->input->name = name;
-		}
-	}
+	if (suffix)
+		hi->input->name = devm_kasprintf(&hdev->dev, GFP_KERNEL,
+						 "%s %s", hdev->name, suffix);
 
 	return 0;
 }
-- 
2.40.1

[PATCH HID 3/3] HID: nvidia-shield: Reference hid_device devm allocation of input_dev name

[Rahul Rameshbabu]

From: Rahul Rameshbabu <rrameshbabu@nvidia.com>

Use hid_device for devm allocation of the input_dev name to avoid a
use-after-free. input_unregister_device would trigger devres cleanup of all
resources associated with the input_dev, free-ing the name. The name would
subsequently be used in a uevent fired at the end of unregistering the
input_dev.

Reported-by: Maxime Ripard <mripard@kernel.org>
Closes: https://lore.kernel.org/linux-input/ZOZIZCND+L0P1wJc@penguin/T/#m443f3dce92520f74b6cf6ffa8653f9c92643d4ae
Fixes: 09308562d4af ("HID: nvidia-shield: Initial driver implementation with Thunderstrike support")
Suggested-by: Maxime Ripard <mripard@kernel.org>
Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
---
 drivers/hid/hid-nvidia-shield.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/hid-nvidia-shield.c b/drivers/hid/hid-nvidia-shield.c
index a928ad2be62d..084179a6be86 100644
--- a/drivers/hid/hid-nvidia-shield.c
+++ b/drivers/hid/hid-nvidia-shield.c
@@ -164,7 +164,7 @@ static struct input_dev *shield_allocate_input_dev(struct hid_device *hdev,
 	idev->id.product = hdev->product;
 	idev->id.version = hdev->version;
 	idev->uniq = hdev->uniq;
-	idev->name = devm_kasprintf(&idev->dev, GFP_KERNEL, "%s %s", hdev->name,
+	idev->name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "%s %s", hdev->name,
 				    name_suffix);
 	if (!idev->name)
 		goto err_name;
-- 
2.40.1

Re: [PATCH HID 0/3] Fix devm references used in HID drivers when allocating input_dev name

[Maxime Ripard]

On Thu, 24 Aug 2023 06:13:52 +0000, Rahul Rameshbabu wrote:
> Maxime Ripard analyzed the following situation involving a use-after-free caused
> by incorrect devres management.
> 
>   1. input_dev name allocated as a resource referring to the same input_dev
>      instance
> 
> [ ... ]

Reviewed-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

Re: [PATCH HID 0/3] Fix devm references used in HID drivers when allocating input_dev name

[Benjamin Tissoires]

On Thu, 24 Aug 2023 06:13:52 +0000, Rahul Rameshbabu wrote:
> Maxime Ripard analyzed the following situation involving a use-after-free caused
> by incorrect devres management.
> 
>   1. input_dev name allocated as a resource referring to the same input_dev
>      instance
>   2. The input_dev is eventually unregistered
>   3. Unregistering the device first involves releasing devres managed resources
>      tied to the input_dev
>   4. A uevent is then fired for the input_dev, referencing various members of
>      the input_dev including the name
>   5. This leads to a use-after-free in the context of the triggered uevent
> 
> [...]

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git (for-6.6/devm-fixes), thanks!

[1/3] HID: uclogic: Correct devm device reference for hidinput input_dev name
      https://git.kernel.org/hid/hid/c/dd613a4e45f8
[2/3] HID: multitouch: Correct devm device reference for hidinput input_dev name
      https://git.kernel.org/hid/hid/c/479439463529
[3/3] HID: nvidia-shield: Reference hid_device devm allocation of input_dev name
      https://git.kernel.org/hid/hid/c/197d3143520f

Cheers,
-- 
Benjamin Tissoires <bentiss@kernel.org>