Paths

Table of Contentst

-
usr.sbin/bluetooth/bthidd/
-
bluetooth/
-
bthidd/
1/6
hid.c

bthidd: Consider usage ranges when dealing with array inputs.
ClosedPublic
Actions

Authored by rakuco on Apr 5 2015, 5:41 PM.

Details

Reviewers

emax
mav

Commits

rS281568: MFC r281116.
rS281566: MFC r281116.
rS281116: bthidd: Consider usage ranges when dealing with array inputs.

Summary

So far, we were always using HID_USAGE() to determine the Usage ID of a
certain HID report input item. This does not work as intended if a field
is an array and the allowed usages are specified with a usage range, as
HID_USAGE() will return 0. We need to use the field value as an index in
the usage range list in this case instead.

This makes the volume keys in a Microsoft Bluetooth Mobile Keyboard
5000 be properly recognized. The relevant part of the HID report looks
like this:

0xA1, 0x01,        // Collection (Application)
0x85, 0x07,        //   Report ID (7)
0x05, 0x0C,        //   Usage Page (Consumer)
0x19, 0x00,        //   Usage Minimum (Unassigned)
0x2A, 0xFF, 0x03,  //   Usage Maximum (0x03FF)
0x95, 0x01,        //   Report Count (1)
0x75, 0x10,        //   Report Size (16)
0x15, 0x00,        //   Logical Minimum (0)
0x27, 0xFF, 0x03, 0x00, 0x00,  //   Logical Maximum (1023)
0x81, 0x00,        //   Input (Data,Array,Abs,No Wrap,Linear,Preferred
                   //   State,No Null Position)

When a key such as "volume down" is pressed, the following data is
transferred through Interrupt In:

0x07 0xEA 0x00

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

rakuco updated this revision to Diff 4680.Apr 5 2015, 5:41 PM

rakuco retitled this revision from to bthidd: Consider usage ranges when dealing with array inputs..

rakuco added reviewers: emax, mav.

rakuco updated this object.

Herald added a subscriber: imp. · View Herald TranscriptApr 5 2015, 5:41 PM

I'd appreciate some feedback here. Should I check for overflows in the code I've added? Does it make sense that calls to hid_get_item() return a single item for an array even if the report count is > 1? The keyboard handling code that manually works around it, but I wonder if this should be done in libusbhid (and in the analogous kernel code) instead.

please see minor inline comment. thank you for your work!

usr.sbin/bluetooth/bthidd/hid.c
182	can you please switch blocks within if() so it looks like if (( h & HIO_VARIABLE)) { usage = HID_USAGE(..); } else { new code; }. its easier to read this way. also usage_offset can be made local (it does not seems be used anywhere else). otherwise, looks fine.

Patch updated with the if check inverted. I thought all variables were supposed to be declared at the top, thankfully it doesn't seem to be the case :-)

Is the code enough as-is or should I add some overflow checks?

emax added inline comments.Apr 5 2015, 5:59 PM

usr.sbin/bluetooth/bthidd/hid.c
177	please add extra set of brackets around h & HIO_VARIABLE, i.e. if ((h & HIO_VARIABLE)). this is to avoid possible warning. can you please explain which overflow are you worrying here about?

Additional ()'s added to the if check.

looks fine to me.

Weird cases that I have in mind:

h.logical_minimum being larger than val.
(h.usage_minimum + usage_offset) > h.usage_maximum.
An array being defined with a usage ID instead of a usage range (all items would have the same usage, which does not make much sense to me and I haven't seen any examples of this so far).

i don't think its worth worrying about. HID_USAGE is basically

#define HID_USAGE(u) ((u) & 0xffff)

so, if HID descriptor and/or value is wrong (i.e. out of range), nothing terrible would happen (imo).

emax accepted this revision.Apr 5 2015, 6:34 PM

emax edited edge metadata.

This revision is now accepted and ready to land.Apr 5 2015, 6:34 PM

Closed by commit rS281116 (authored by @rakuco).

Revision Contents
Changeset List

Path

Size

usr.sbin/

bluetooth/

bthidd/

hid.c

15 lines

Diff 4680

View Options

usr.sbin/bluetooth/bthidd/hid.c

Show First 20 Lines • Show All 82 Lines • ▼ Show 20 Lines
*		*
* The environment variable vfs.root.mountfrom.options is a comma delimited		* The environment variable vfs.root.mountfrom.options is a comma delimited
* set of string mount options. These mount options must be parseable		* set of string mount options. These mount options must be parseable
* by nmount() in the kernel.		* by nmount() in the kernel.
*/		*/

static int parse_mount(char **);		static int parse_mount(char **);
static struct mntarg parse_mountroot_options(struct mntarg , const char *);		static struct mntarg parse_mountroot_options(struct mntarg , const char *);
		static int vfs_mountroot_wait_if_neccessary(const char fs, const char dev);

/*		/*
* The vnode of the system's root (/ in the filesystem, without chroot		* The vnode of the system's root (/ in the filesystem, without chroot
* active.)		* active.)
*/		*/
struct vnode *rootvnode;		struct vnode *rootvnode;

/*		/*
▲ Show 20 Lines • Show All 579 Lines • ▼ Show 20 Lines	parse_mount_dev_present(const char *dev)
struct nameidata nd;		struct nameidata nd;
int error;		int error;

NDINIT(&nd, LOOKUP, FOLLOW \| LOCKLEAF, UIO_SYSSPACE, dev, curthread);		NDINIT(&nd, LOOKUP, FOLLOW \| LOCKLEAF, UIO_SYSSPACE, dev, curthread);
error = namei(&nd);		error = namei(&nd);
if (!error)		if (!error)
vput(nd.ni_vp);		vput(nd.ni_vp);
NDFREE(&nd, NDF_ONLY_PNBUF);		NDFREE(&nd, NDF_ONLY_PNBUF);
		if (error != 0)
		printf("%s: %s not present, error %d\n", __func__, dev, error);
		marcelUnsubmitted Done Inline Actions This looks like it's more for debugging than for an end user. Maybe put this under boot verbose? The reason is that it's not an error until it's an error, meaning that if we're going to wait for the device when it's not there yet, then printing errors is confusing. Putting it under boot verbose is a nice way for people to get insight into why a boot may be stalled... marcel: This looks like it's more for debugging than for an end user. Maybe put this under boot verbose?
return (error != 0) ? 0 : 1;		return (error != 0) ? 0 : 1;
}		}
		traszAuthorUnsubmitted Not Done Inline Actions Actually, it was a debugging printf that snuck in. Removed. trasz: Actually, it was a debugging printf that snuck in. Removed.

#define ERRMSGL 255		#define ERRMSGL 255
static int		static int
parse_mount(char **conf)		parse_mount(char **conf)
{		{
char *errmsg;		char *errmsg;
struct mntarg *ma;		struct mntarg *ma;
char dev, fs, opts, tok;		char dev, fs, opts, tok;
int delay, error, timeout;		int error;

error = parse_token(conf, &tok);		error = parse_token(conf, &tok);
if (error)		if (error)
return (error);		return (error);
fs = tok;		fs = tok;
error = parse_skipto(&tok, ':');		error = parse_skipto(&tok, ':');
if (error) {		if (error) {
free(fs, M_TEMP);		free(fs, M_TEMP);
Show All 20 Lines	parse_mount(char **conf)
errmsg = malloc(ERRMSGL, M_TEMP, M_WAITOK \| M_ZERO);		errmsg = malloc(ERRMSGL, M_TEMP, M_WAITOK \| M_ZERO);

if (vfs_byname(fs) == NULL) {		if (vfs_byname(fs) == NULL) {
strlcpy(errmsg, "unknown file system", ERRMSGL);		strlcpy(errmsg, "unknown file system", ERRMSGL);
error = ENOENT;		error = ENOENT;
goto out;		goto out;
}		}

if (strcmp(fs, "zfs") != 0 && strstr(fs, "nfs") == NULL &&		error = vfs_mountroot_wait_if_neccessary(fs, dev);
dev[0] != '\0' && !parse_mount_dev_present(dev)) {		if (error != 0)
printf("mountroot: waiting for device %s ...\n", dev);
delay = hz / 10;
timeout = root_mount_timeout * hz;
do {
pause("rmdev", delay);
timeout -= delay;
} while (timeout > 0 && !parse_mount_dev_present(dev));
if (timeout <= 0) {
error = ENODEV;
goto out;		goto out;
}
}

ma = NULL;		ma = NULL;
ma = mount_arg(ma, "fstype", fs, -1);		ma = mount_arg(ma, "fstype", fs, -1);
ma = mount_arg(ma, "fspath", "/", -1);		ma = mount_arg(ma, "fspath", "/", -1);
ma = mount_arg(ma, "from", dev, -1);		ma = mount_arg(ma, "from", dev, -1);
ma = mount_arg(ma, "errmsg", errmsg, ERRMSGL);		ma = mount_arg(ma, "errmsg", errmsg, ERRMSGL);
ma = mount_arg(ma, "ro", NULL, 0);		ma = mount_arg(ma, "ro", NULL, 0);
ma = parse_mountroot_options(ma, opts);		ma = parse_mountroot_options(ma, opts);
▲ Show 20 Lines • Show All 191 Lines • ▼ Show 20 Lines	if (ppsratecheck(&lastfail, &curfail, 1)) {
printf(" %s", h->who);		printf(" %s", h->who);
printf("\n");		printf("\n");
}		}
msleep(&root_holds, &root_holds_mtx, PZERO \| PDROP, "roothold",		msleep(&root_holds, &root_holds_mtx, PZERO \| PDROP, "roothold",
hz);		hz);
}		}
}		}

		static int
		vfs_mountroot_wait_if_neccessary(const char fs, const char dev)
		kibUnsubmitted Not Done Inline Actions Please come with much shorter name for the function. kib: Please come with much shorter name for the function.
		{
		int delay, timeout;

		/*
		* In case of ZFS and NFS we don't have a way to wait for
		* specific device.
		*/
		if (strcmp(fs, "zfs") == 0 \|\| strstr(fs, "nfs") != NULL \|\|
		dev[0] == '\0') {
		vfs_mountroot_wait();
		return (0);
		}

		/*
		* Otherwise, no point in waiting if the device is already there.
		* Note that we must wait for GEOM to finish reconfiguring itself,
		* eg for geom_part(4) to finish tasting.
		*/
		DROP_GIANT();
		g_waitidle();
		kibUnsubmitted Not Done Inline Actions I think this is not needed, and probably even dangerous, for the nfs case. I have no better suggestion than ugly strstr("nfs") hack, but it is still much better than preventing nfs mount due to the issues with the local storage subsystem. kib: I think this is not needed, and probably even dangerous, for the nfs case. I have no better…
		traszAuthorUnsubmitted Not Done Inline Actions For the non-NFS case it's definitely needed, because without it, partitions don't get created before we check for the device presence, and we end up doing the full wait in all cases, defying the purpose of this patch. For the NFS case, this code is not reached - just like for ZFS, we call vfs_mountroot_wait() and return few lines earlier. trasz: For the non-NFS case it's definitely needed, because without it, partitions don't get created…
		PICKUP_GIANT();
		if (parse_mount_dev_present(dev))
		return (0);

		/*
		* No luck. Let's wait. This code looks weird, but it's that way
		* to behave exactly as it used to work before.
		*/
		vfs_mountroot_wait();
		printf("mountroot: waiting for device %s...\n", dev);
		delay = hz / 10;
		timeout = root_mount_timeout * hz;
		do {
		pause("rmdev", delay);
		timeout -= delay;
		} while (timeout > 0 && !parse_mount_dev_present(dev));

		if (timeout <= 0)
		return (ENODEV);

		kibUnsubmitted Not Done Inline Actions This blank line is not needed. kib: This blank line is not needed.
		return (0);
		}

void		void
vfs_mountroot(void)		vfs_mountroot(void)
{		{
struct mount *mp;		struct mount *mp;
struct sbuf *sb;		struct sbuf *sb;
struct thread *td;		struct thread *td;
time_t timebase;		time_t timebase;
int error;		int error;

td = curthread;		td = curthread;

vfs_mountroot_wait();

sb = sbuf_new_auto();		sb = sbuf_new_auto();
vfs_mountroot_conf0(sb);		vfs_mountroot_conf0(sb);
sbuf_finish(sb);		sbuf_finish(sb);

error = vfs_mountroot_devfs(td, &mp);		error = vfs_mountroot_devfs(td, &mp);
while (!error) {		while (!error) {
error = vfs_mountroot_parse(sb, mp);		error = vfs_mountroot_parse(sb, mp);
▲ Show 20 Lines • Show All 88 Lines • Show Last 20 Lines

bthidd: Consider usage ranges when dealing with array inputs.ClosedPublicActions