From: joshua stein Subject: Re: signedness fixes in hid(4) To: tech@openbsd.org Date: Sun, 26 Oct 2025 10:53:56 -0500 I didn't get a definitive answer on whether we want this when I sent it months ago but it is needed for certain devices. ok? commit e6433115cb58faed8b4b68507d39c984e798a5b9 Author: joshua stein Date: Thu May 1 13:32:30 2025 -0500 hid: properly handle signed values We were formerly parsing descriptor data like this: 0x15, 0x81, // Logical Minimum (129) 0x25, 0x7F, // Logical Maximum (127) when it should be a signed value: 0x15, 0x81, // Logical Minimum (-127) 0x25, 0x7F, // Logical Maximum (127) This caused issues comparing values like x/y min/max. NetBSD made a similar change in 2018 diff --git sys/dev/hid/hid.c sys/dev/hid/hid.c index e6203a7483f..8ed667ba5ac 100644 --- sys/dev/hid/hid.c +++ sys/dev/hid/hid.c @@ -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; @@ -192,8 +192,7 @@ hid_get_item(struct hid_data *s, struct hid_item *h) { struct hid_item *c; unsigned int bTag, bType, bSize; - uint32_t oldpos; - int32_t mask; + uint32_t oldpos, uval; int32_t dval; if (s == NULL) @@ -249,7 +248,6 @@ hid_get_item(struct hid_data *s, struct hid_item *h) /* get next item */ while (s->p != s->end) { - bSize = hid_get_byte(s, 1); if (bSize == 0xfe) { /* long item */ @@ -267,34 +265,34 @@ hid_get_item(struct hid_data *s, struct hid_item *h) } switch (bSize) { case 0: - dval = 0; - mask = 0; + uval = 0; + dval = uval; break; case 1: - dval = hid_get_byte(s, 1); - mask = 0xFF; + uval = hid_get_byte(s, 1); + dval = (int8_t)uval; break; case 2: - dval = hid_get_byte(s, 1); - dval |= hid_get_byte(s, 1) << 8; - mask = 0xFFFF; + uval = hid_get_byte(s, 1); + uval |= hid_get_byte(s, 1) << 8; + dval = (int16_t)uval; 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; - mask = 0xFFFFFFFF; + 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 = (int32_t)uval; break; default: - dval = hid_get_byte(s, bSize); + uval = hid_get_byte(s, bSize); DPRINTF("bad length %u (data=0x%02x)\n", - bSize, dval); + bSize, uval); continue; } - DPRINTF("%s: bType=%d bTag=%d dval=%d\n", __func__, - bType, bTag, dval); + DPRINTF("%s: bType=%d bTag=%d dval=%d uval=%u\n", __func__, + bType, bTag, dval, uval); switch (bType) { case 0: /* Main */ switch (bTag) { @@ -330,7 +328,7 @@ hid_get_item(struct hid_data *s, struct hid_item *h) 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 +354,7 @@ hid_get_item(struct hid_data *s, struct hid_item *h) 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 +369,19 @@ hid_get_item(struct hid_data *s, struct hid_item *h) 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; break; case 8: - hid_switch_rid(s, c, dval & mask); + hid_switch_rid(s, c, dval); break; case 9: - /* mask because value is unsigned */ - s->loc_count = dval & mask; + s->loc_count = uval; break; case 10: /* Push */ if (s->pushlevel < MAXPUSH - 1) { @@ -428,14 +424,14 @@ hid_get_item(struct hid_data *s, struct hid_item *h) switch (bTag) { case 0: if (bSize != 4) - dval = (dval & mask) | c->_usage_page; + uval = c->_usage_page | uval; /* 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 +444,16 @@ hid_get_item(struct hid_data *s, struct hid_item *h) s->susage |= 1; if (bSize != 4) - dval = (dval & mask) | c->_usage_page; - c->usage_minimum = dval; + uval = c->_usage_page | uval; + 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 = c->_usage_page | uval; + c->usage_maximum = uval; check_set: if (s->susage != 3) @@ -478,25 +474,25 @@ hid_get_item(struct hid_data *s, struct hid_item *h) 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); @@ -545,7 +541,7 @@ hid_report_size(const void *buf, int len, enum hid_kind k, u_int8_t id) } int -hid_locate(const void *desc, int size, int32_t u, uint8_t id, enum hid_kind k, +hid_locate(const void *desc, int size, uint32_t u, uint8_t id, enum hid_kind k, struct hid_location *loc, uint32_t *flags) { struct hid_data *d; @@ -683,7 +679,7 @@ hid_get_collection_data(const void *desc, int size, int32_t usage, } int -hid_get_id_of_collection(const void *desc, int size, int32_t usage, +hid_get_id_of_collection(const void *desc, int size, uint32_t usage, uint32_t collection) { struct hid_data *hd; diff --git sys/dev/hid/hid.h sys/dev/hid/hid.h index 7f4d38f6f74..014c7526734 100644 --- sys/dev/hid/hid.h +++ sys/dev/hid/hid.h @@ -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; @@ -88,13 +88,13 @@ struct hid_data *hid_start_parse(const void *, int, enum hid_kind); void hid_end_parse(struct hid_data *); int hid_get_item(struct hid_data *, struct hid_item *); int hid_report_size(const void *, int, enum hid_kind, uint8_t); -int hid_locate(const void *, int, int32_t, uint8_t, enum hid_kind, +int hid_locate(const void *, int, uint32_t, uint8_t, enum hid_kind, struct hid_location *, uint32_t *); int32_t hid_get_data(const uint8_t *buf, int, struct hid_location *); uint32_t hid_get_udata(const uint8_t *buf, int, struct hid_location *); int hid_is_collection(const void *, int, uint8_t, int32_t); struct hid_data *hid_get_collection_data(const void *, int, int32_t, uint32_t); -int hid_get_id_of_collection(const void *, int, int32_t, uint32_t); +int hid_get_id_of_collection(const void *, int, uint32_t, uint32_t); int hid_find_report(const void *, int len, enum hid_kind, int32_t, int, int32_t *, int32_t *); diff --git sys/dev/hid/hidmtvar.h sys/dev/hid/hidmtvar.h index 1b799bff875..724bff0e1cb 100644 --- sys/dev/hid/hidmtvar.h +++ sys/dev/hid/hidmtvar.h @@ -16,7 +16,7 @@ */ struct hidmt_data { - int32_t usage; + uint32_t usage; struct hid_location loc; SIMPLEQ_ENTRY(hidmt_data) entry; }; diff --git sys/dev/i2c/ihidev.c sys/dev/i2c/ihidev.c index c922b78573b..ac07767fad5 100644 --- sys/dev/i2c/ihidev.c +++ sys/dev/i2c/ihidev.c @@ -799,7 +799,7 @@ ihidev_maxrepid(void *buf, int len) maxid = -1; h.report_ID = 0; for (d = hid_start_parse(buf, len, hid_all); hid_get_item(d, &h);) - if (h.report_ID > maxid) + if ((int)h.report_ID > maxid) maxid = h.report_ID; hid_end_parse(d); diff --git sys/dev/usb/uhidev.c sys/dev/usb/uhidev.c index 37f60cb8b53..83b681a12bf 100644 --- sys/dev/usb/uhidev.c +++ sys/dev/usb/uhidev.c @@ -402,7 +402,7 @@ uhidev_maxrepid(void *buf, int len) maxid = -1; h.report_ID = 0; for (d = hid_start_parse(buf, len, hid_all); hid_get_item(d, &h);) - if (h.report_ID > maxid) + if ((int)h.report_ID > maxid) maxid = h.report_ID; hid_end_parse(d); return (maxid);