Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 898e69ca authored by Jann Horn's avatar Jann Horn Committed by Greg Kroah-Hartman
Browse files

HID: uhid: Fix worker destroying device without any protection 



commit 4ea5763fb79ed89b3bdad455ebf3f33416a81624 upstream.

uhid has to run hid_add_device() from workqueue context while allowing
parallel use of the userspace API (which is protected with ->devlock).
But hid_add_device() can fail. Currently, that is handled by immediately
destroying the associated HID device, without using ->devlock - but if
there are concurrent requests from userspace, that's wrong and leads to
NULL dereferences and/or memory corruption (via use-after-free).

Fix it by leaving the HID device as-is in the worker. We can clean it up
later, either in the UHID_DESTROY command handler or in the ->release()
handler.

Cc: stable@vger.kernel.org
Fixes: 67f8ecc5 ("HID: uhid: fix timeout when probe races with IO")
Signed-off-by: default avatarJann Horn <jannh@google.com>
Signed-off-by: default avatarJiri Kosina <jkosina@suse.cz>
Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
parent 4aa2e739

drivers/hid/uhid.c

+25 −4
Original line number Diff line number Diff line
@@ -28,11 +28,22 @@ 


struct uhid_device {

	struct mutex devlock;



	/* This flag tracks whether the HID device is usable for commands from

	 * userspace. The flag is already set before hid_add_device(), which

	 * runs in workqueue context, to allow hid_add_device() to communicate

	 * with userspace.

	 * However, if hid_add_device() fails, the flag is cleared without

	 * holding devlock.

	 * We guarantee that if @running changes from true to false while you're

	 * holding @devlock, it's still fine to access @hid.

	 */

	bool running;



	__u8 *rd_data;

	uint rd_size;



	/* When this is NULL, userspace may use UHID_CREATE/UHID_CREATE2. */

	struct hid_device *hid;

	struct uhid_event input_buf;
@@ -63,9 +74,18 @@ static void uhid_device_add_worker(struct work_struct *work) 
	if (ret) {

		hid_err(uhid->hid, "Cannot register HID device: error %d\n", ret);



		hid_destroy_device(uhid->hid);

		uhid->hid = NULL;

		/* We used to call hid_destroy_device() here, but that's really

		 * messy to get right because we have to coordinate with

		 * concurrent writes from userspace that might be in the middle

		 * of using uhid->hid.

		 * Just leave uhid->hid as-is for now, and clean it up when

		 * userspace tries to close or reinitialize the uhid instance.

		 *

		 * However, we do have to clear the ->running flag and do a

		 * wakeup to make sure userspace knows that the device is gone.

		 */

		uhid->running = false;

		wake_up_interruptible(&uhid->report_wait);

	}

}
@@ -474,7 +494,7 @@ static int uhid_dev_create2(struct uhid_device *uhid, 
	void *rd_data;

	int ret;



	if (uhid->running)

	if (uhid->hid)

		return -EALREADY;



	rd_size = ev->u.create2.rd_size;
@@ -556,7 +576,7 @@ static int uhid_dev_create(struct uhid_device *uhid, 


static int uhid_dev_destroy(struct uhid_device *uhid)

{

	if (!uhid->running)

	if (!uhid->hid)

		return -EINVAL;



	uhid->running = false;
@@ -565,6 +585,7 @@ static int uhid_dev_destroy(struct uhid_device *uhid) 
	cancel_work_sync(&uhid->worker);



	hid_destroy_device(uhid->hid);

	uhid->hid = NULL;

	kfree(uhid->rd_data);



	return 0;