Download raw body.
signedness fixes in hid(4)
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 <jcs@jcs.org>
> 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);
signedness fixes in hid(4)