[PATCH 1/2] HID: check empty report_list in hid_validate_values()

[PATCH 1/2] HID: check empty report_list in hid_validate_values()

[Pietro Borrello]

Add a check for empty report_list in hid_validate_values().
The missing check causes a type confusion when issuing a list_entry()
on an empty report_list.
The problem is caused by the assumption that the device must
have valid report_list. While this will be true for all normal HID
devices, a suitably malicious device can violate the assumption.

Fixes: 1b15d2e5b807 ("HID: core: fix validation of report id 0")
Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it>
---
 drivers/hid/hid-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index bd47628da6be..3e1803592bd4 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -993,8 +993,8 @@ struct hid_report *hid_validate_values(struct hid_device *hid,
 		 * Validating on id 0 means we should examine the first
 		 * report in the list.
 		 */
-		report = list_entry(
-				hid->report_enum[type].report_list.next,
+		report = list_first_entry_or_null(
+				&hid->report_enum[type].report_list,
 				struct hid_report, list);
 	} else {
 		report = hid->report_enum[type].report_id_hash[id];

-- 
2.25.1

[PATCH 0/2] Cover Letter: HID: drop assumptions on non-empty report lists

[Pietro Borrello]

We found potential misuses of list_entry() on lists in hid driver
code that are not checked.
Issuing a list_entry() on an empty list causes a type confusion making
the list_entry point to the list_head itself.
The most impactful seems the missing check for an empty list in
hid_validate_values() which is supposed to check the validity of the
reports themselves, potentially affecting all the drivers that rely on it.

The problem is caused by the driver's assumption that the device must
have valid report_list. While this will be true for all normal HID
devices, a suitably malicious device can violate the assumption.

This patch fixes the issue by checking that the lists are nonempty
before allowing them to be used.

At a first glance it may seem that the patches have security implications.
However, when plugging a device which provides a descriptor with no output
report, the type confusions will create a fake struct hid_report*
which points to ((struct hid_device *)hid).report_enum[type].report_list.
This, by chance, makes the type confused structure to span
the `struct hid_report* report_id_hash[256]` array in the
((struct hid_device *)hid).report_enum[type] field.

Then, due to their semantics hid_validate_values() will check
(report->maxfield > field_index) on the type-confused report,
and the maxfield field happens to overlap on the
report_id_hash[] array in the report_enum[type] field
which are all NULL since we provided no reports.
Similarly, for bigben_probe(), the confused report entry is
used in the bigben_worker() function which checks
(report->field[0] != NULL) that, again, overlaps with a NULL
pointer.
It seems there is a commit (918aa1ef104d: "HID: bigbenff: prevent
null pointer dereference") which added the check for report_field
being NULL to bigben_worker() to prevent crashing, but without
checking the actual root cause.

Thus, while being type confusions bugs, they are not exploitable.
I still believe list checks should be added, and the patches on
hid_validate_values() and bigben_probe() merged,
to prevent future exploitability if the shape of the structure changes,
and they do not overlap anymore with NULL pointers.
In this case, it is not exploitable just by the pure chance of struct
member ordering.

To: Jiri Kosina <jikos@kernel.org>
To: Benjamin Tissoires <benjamin.tissoires@redhat.com>
To: Kees Cook <keescook@chromium.org>
To: Hanno Zulla <abos@hanno.de>
Cc: Cristiano Giuffrida <c.giuffrida@vu.nl>
Cc: "Bos, H.J." <h.j.bos@vu.nl>
Cc: Jakob Koschel <jkl820.git@gmail.com>
Cc: Jiri Kosina <jkosina@suse.cz>
Cc: linux-input@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it>

---
Pietro Borrello (2):
      HID: check empty report_list in hid_validate_values()
      HID: check empty report_list in bigben_probe()

 drivers/hid/hid-bigbenff.c | 5 +++++
 drivers/hid/hid-core.c     | 4 ++--
 2 files changed, 7 insertions(+), 2 deletions(-)
---
base-commit: 5dc4c995db9eb45f6373a956eb1f69460e69e6d4
change-id: 20230114-hid-fix-emmpty-report-list-0d9ab58b234d

Best regards,
-- 
Pietro Borrello <borrello@diag.uniroma1.it>

[PATCH 2/2] HID: check empty report_list in bigben_probe()

[Pietro Borrello]

Add a check for empty report_list in bigben_probe().
The missing check causes a type confusion when issuing a list_entry()
on an empty report_list.
The problem is caused by the assumption that the device must
have valid report_list. While this will be true for all normal HID
devices, a suitably malicious device can violate the assumption.

Fixes: 256a90ed9e46 ("HID: hid-bigbenff: driver for BigBen Interactive PS3OFMINIPAD gamepad")
Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it>
---
 drivers/hid/hid-bigbenff.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/hid/hid-bigbenff.c b/drivers/hid/hid-bigbenff.c
index e8c5e3ac9fff..e8b16665860d 100644
--- a/drivers/hid/hid-bigbenff.c
+++ b/drivers/hid/hid-bigbenff.c
@@ -344,6 +344,11 @@ static int bigben_probe(struct hid_device *hid,
 	}
 
 	report_list = &hid->report_enum[HID_OUTPUT_REPORT].report_list;
+	if (list_empty(report_list)) {
+		hid_err(hid, "no output report found\n");
+		error = -ENODEV;
+		goto error_hw_stop;
+	}
 	bigben->report = list_entry(report_list->next,
 		struct hid_report, list);
 

-- 
2.25.1

Re: [PATCH 0/2] Cover Letter: HID: drop assumptions on non-empty report lists

[Jiri Kosina]

On Mon, 16 Jan 2023, Pietro Borrello wrote:

> We found potential misuses of list_entry() on lists in hid driver
> code that are not checked.
> Issuing a list_entry() on an empty list causes a type confusion making
> the list_entry point to the list_head itself.
> The most impactful seems the missing check for an empty list in
> hid_validate_values() which is supposed to check the validity of the
> reports themselves, potentially affecting all the drivers that rely on it.

Both applied to hid.git#for-6.2/upstream-fixes. Thanks Pietro,

-- 
Jiri Kosina
SUSE Labs