From: "Jonathan Armani" Subject: Re: signedness fixes in hid(4) To: tech@openbsd.org Date: Sun, 26 Oct 2025 22:58:22 +0100 Hi, Looks good to me, will give it a try with my HID devices this week but ok @armani if you have another test. On Sun, Oct 26, 2025, at 16:53, joshua stein wrote: > 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);