Index | Thread | Search

From:
Miod Vallat <miod@online.fr>
Subject:
signedness fixes in hid(4)
To:
tech@openbsd.org
Date:
Sun, 4 May 2025 19:48:40 +0000

Download raw body.

Thread
This is NetBSD PR#53605 via FreeBSD:
https://cgit.freebsd.org/src/commit/?id=38b67578fb4bbf568f7012ca3921a4d15cfe7c5d

It looks like some devices have report descriptors which causes us to
compute negative array offsets due to sign extension.

Please test and report any regression.

Index: sys/dev/hid/hid.c
===================================================================
RCS file: /OpenBSD/src/sys/dev/hid/hid.c,v
diff -u -p -u -p -r1.7 hid.c
--- sys/dev/hid/hid.c	10 May 2024 10:49:10 -0000	1.7
+++ sys/dev/hid/hid.c	4 May 2025 19:37:42 -0000
@@ -49,7 +49,7 @@
 #define	MAXID 16
 
 struct hid_pos_data {
-	int32_t rid;
+	uint32_t rid;
 	uint32_t pos;
 };
 
@@ -59,9 +59,9 @@ struct hid_data {
 	const uint8_t *p;
 	struct hid_item cur[MAXPUSH];
 	struct hid_pos_data last_pos[MAXID];
-	int32_t	usages_min[MAXUSAGE];
-	int32_t	usages_max[MAXUSAGE];
-	int32_t usage_last;	/* last seen usage */
+	uint32_t usages_min[MAXUSAGE];
+	uint32_t usages_max[MAXUSAGE];
+	uint32_t usage_last;	/* last seen usage */
 	uint32_t loc_size;	/* last seen size */
 	uint32_t loc_count;	/* last seen count */
 	enum hid_kind kind;
@@ -92,7 +92,7 @@ hid_clear_local(struct hid_item *c)
 }
 
 static void
-hid_switch_rid(struct hid_data *s, struct hid_item *c, int32_t nextid)
+hid_switch_rid(struct hid_data *s, struct hid_item *c, uint32_t nextid)
 {
 	uint8_t i;
 
@@ -195,6 +195,7 @@ hid_get_item(struct hid_data *s, struct 
 	uint32_t oldpos;
 	int32_t mask;
 	int32_t dval;
+	uint32_t uval;
 
 	if (s == NULL)
 		return (0);
@@ -211,10 +212,10 @@ hid_get_item(struct hid_data *s, struct 
 	if (s->icount < s->ncount) {
 		/* get current usage */
 		if (s->iusage < s->nusage) {
-			dval = s->usages_min[s->iusage] + s->ousage;
-			c->usage = dval;
-			s->usage_last = dval;
-			if (dval == s->usages_max[s->iusage]) {
+			uval = s->usages_min[s->iusage] + s->ousage;
+			c->usage = uval;
+			s->usage_last = uval;
+			if (uval == s->usages_max[s->iusage]) {
 				s->iusage ++;
 				s->ousage = 0;
 			} else {
@@ -222,7 +223,7 @@ hid_get_item(struct hid_data *s, struct 
 			}
 		} else {
 			DPRINTF("Using last usage\n");
-			dval = s->usage_last;
+			uval = s->usage_last;
 		}
 		s->icount ++;
 		/*
@@ -267,29 +268,34 @@ hid_get_item(struct hid_data *s, struct 
 		}
 		switch (bSize) {
 		case 0:
-			dval = 0;
+			uval = 0;
+			dval = uval;
 			mask = 0;
 			break;
 		case 1:
-			dval = hid_get_byte(s, 1);
+			uval = hid_get_byte(s, 1);
+			dval = (int8_t)uval;
 			mask = 0xFF;
 			break;
 		case 2:
-			dval = hid_get_byte(s, 1);
-			dval |= hid_get_byte(s, 1) << 8;
+			uval = hid_get_byte(s, 1);
+			uval |= hid_get_byte(s, 1) << 8;
+			dval = (int16_t)uval;
 			mask = 0xFFFF;
 			break;
 		case 4:
-			dval = hid_get_byte(s, 1);
-			dval |= hid_get_byte(s, 1) << 8;
-			dval |= hid_get_byte(s, 1) << 16;
-			dval |= hid_get_byte(s, 1) << 24;
+			uval = hid_get_byte(s, 1);
+			uval |= hid_get_byte(s, 1) << 8;
+			uval |= hid_get_byte(s, 1) << 16;
+			uval |= hid_get_byte(s, 1) << 24;
+			dval = uval;
 			mask = 0xFFFFFFFF;
 			break;
 		default:
-			dval = hid_get_byte(s, bSize);
+			uval = hid_get_byte(s, bSize);
+			dval = uval;
 			DPRINTF("bad length %u (data=0x%02x)\n",
-			    bSize, dval);
+			    bSize, uval);
 			continue;
 		}
 
@@ -300,7 +306,7 @@ hid_get_item(struct hid_data *s, struct 
 			switch (bTag) {
 			case 8:	/* Input */
 				c->kind = hid_input;
-				c->flags = dval;
+				c->flags = uval;
 		ret:
 				c->loc.count = s->loc_count;
 				c->loc.size = s->loc_size;
@@ -326,11 +332,11 @@ hid_get_item(struct hid_data *s, struct 
 
 			case 9:	/* Output */
 				c->kind = hid_output;
-				c->flags = dval;
+				c->flags = uval;
 				goto ret;
 			case 10:	/* Collection */
 				c->kind = hid_collection;
-				c->collection = dval;
+				c->collection = uval;
 				c->collevel++;
 				c->usage = s->usage_last;
 				*h = *c;
@@ -356,7 +362,7 @@ hid_get_item(struct hid_data *s, struct 
 		case 1:		/* Global */
 			switch (bTag) {
 			case 0:
-				c->_usage_page = dval << 16;
+				c->_usage_page = uval << 16;
 				break;
 			case 1:
 				c->logical_minimum = dval;
@@ -371,21 +377,21 @@ hid_get_item(struct hid_data *s, struct 
 				c->physical_maximum = dval;
 				break;
 			case 5:
-				c->unit_exponent = dval;
+				c->unit_exponent = uval;
 				break;
 			case 6:
-				c->unit = dval;
+				c->unit = uval;
 				break;
 			case 7:
 				/* mask because value is unsigned */
-				s->loc_size = dval & mask;
+				s->loc_size = uval & mask;
 				break;
 			case 8:
-				hid_switch_rid(s, c, dval & mask);
+				hid_switch_rid(s, c, uval & mask);
 				break;
 			case 9:
 				/* mask because value is unsigned */
-				s->loc_count = dval & mask;
+				s->loc_count = uval & mask;
 				break;
 			case 10:	/* Push */
 				if (s->pushlevel < MAXPUSH - 1) {
@@ -428,14 +434,14 @@ hid_get_item(struct hid_data *s, struct 
 			switch (bTag) {
 			case 0:
 				if (bSize != 4)
-					dval = (dval & mask) | c->_usage_page;
+					uval = (uval & mask) | c->_usage_page;
 
 				/* set last usage, in case of a collection */
-				s->usage_last = dval;
+				s->usage_last = uval;
 
 				if (s->nusage < MAXUSAGE) {
-					s->usages_min[s->nusage] = dval;
-					s->usages_max[s->nusage] = dval;
+					s->usages_min[s->nusage] = uval;
+					s->usages_max[s->nusage] = uval;
 					s->nusage ++;
 				} else {
 					DPRINTF("max usage reached\n");
@@ -448,16 +454,16 @@ hid_get_item(struct hid_data *s, struct 
 				s->susage |= 1;
 
 				if (bSize != 4)
-					dval = (dval & mask) | c->_usage_page;
-				c->usage_minimum = dval;
+					uval = (uval & mask) | c->_usage_page;
+				c->usage_minimum = uval;
 
 				goto check_set;
 			case 2:
 				s->susage |= 2;
 
 				if (bSize != 4)
-					dval = (dval & mask) | c->_usage_page;
-				c->usage_maximum = dval;
+					uval = (uval & mask) | c->_usage_page;
+				c->usage_maximum = uval;
 
 			check_set:
 				if (s->susage != 3)
@@ -478,25 +484,25 @@ hid_get_item(struct hid_data *s, struct 
 				s->susage = 0;
 				break;
 			case 3:
-				c->designator_index = dval;
+				c->designator_index = uval;
 				break;
 			case 4:
-				c->designator_minimum = dval;
+				c->designator_minimum = uval;
 				break;
 			case 5:
-				c->designator_maximum = dval;
+				c->designator_maximum = uval;
 				break;
 			case 7:
-				c->string_index = dval;
+				c->string_index = uval;
 				break;
 			case 8:
-				c->string_minimum = dval;
+				c->string_minimum = uval;
 				break;
 			case 9:
-				c->string_maximum = dval;
+				c->string_maximum = uval;
 				break;
 			case 10:
-				c->set_delimiter = dval;
+				c->set_delimiter = uval;
 				break;
 			default:
 				DPRINTF("Local bTag=%d\n", bTag);
Index: sys/dev/hid/hid.h
===================================================================
RCS file: /OpenBSD/src/sys/dev/hid/hid.h,v
diff -u -p -u -p -r1.11 hid.h
--- sys/dev/hid/hid.h	12 Aug 2023 20:47:06 -0000	1.11
+++ sys/dev/hid/hid.h	4 May 2025 19:37:42 -0000
@@ -54,27 +54,27 @@ struct hid_location {
 
 struct hid_item {
 	/* Global */
-	int32_t _usage_page;
+	uint32_t _usage_page;
 	int32_t logical_minimum;
 	int32_t logical_maximum;
 	int32_t physical_minimum;
 	int32_t physical_maximum;
-	int32_t unit_exponent;
-	int32_t unit;
-	int32_t report_ID;
+	uint32_t unit_exponent;
+	uint32_t unit;
+	uint32_t report_ID;
 	/* Local */
-	int32_t usage;
-	int32_t usage_minimum;
-	int32_t usage_maximum;
-	int32_t designator_index;
-	int32_t designator_minimum;
-	int32_t designator_maximum;
-	int32_t string_index;
-	int32_t string_minimum;
-	int32_t string_maximum;
-	int32_t set_delimiter;
+	uint32_t usage;
+	uint32_t usage_minimum;
+	uint32_t usage_maximum;
+	uint32_t designator_index;
+	uint32_t designator_minimum;
+	uint32_t designator_maximum;
+	uint32_t string_index;
+	uint32_t string_minimum;
+	uint32_t string_maximum;
+	uint32_t set_delimiter;
 	/* Misc */
-	int32_t collection;
+	uint32_t collection;
 	int collevel;
 	enum hid_kind kind;
 	u_int32_t flags;