Page MenuHomeFreeBSD

evdev: add sysctls with device info
ClosedPublic

Authored by greg_unrelenting.technology on Dec 30 2018, 9:37 PM.

Details

Summary

A big security advantage of Wayland is not allowing applications to read input devices all the time. Having /dev/input/* accessible to the user account subverts this advantage.

libudev-devd was opening the evdev devices to detect their types (mouse, keyboard, touchpad, etc). This does not work when /dev/input/* is inaccessible.

With the kernel exposing this information as sysctls, we can work without /dev/input/* access, preserving the Wayland security model.

p.s. would be nice to see this MFC to 12

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

imp accepted this revision.Dec 30 2018, 11:16 PM

This looks fine, but I'd create a sysctl that publishes this information as well. devd events are just that: one shot events that say that something has changed. when we restart devd, we don't republish everything that's gone before. This looks necessary to cope with devices that are plugged into the system. Otherwise we may have a race if devd starts, processes events, then x11 and friends start and expect this event in the queue. Maybe it is, maybe it isn't. devd is designed to have no memory, and I really really want to keep it that way.

This revision is now accepted and ready to land.Dec 30 2018, 11:16 PM
wulf requested changes to this revision.Dec 31 2018, 4:28 AM
wulf added a subscriber: wulf.

I am objecting to EvdevProbe()-derived part (device type autodetection). It is better to add a KPI to set device type directly rather then via execution of libmagic. We have about 10 evdev drivers in source tree so it's possible to update them all. EvdevProbe()-derived part could be used as (deprecated) fallback option in that case.

This revision now requires changes to proceed.Dec 31 2018, 4:28 AM
In D18694#398917, @imp wrote:

This looks fine, but I'd create a sysctl that publishes this information as well.

Right, sysctl sounds like a good "memory" type thing. I thought they wouldn't be readable in capability mode, but there's something called CTLFLAG_CAPRD.

(the advantage of remembering this information in a daemon would be that an explicit descriptor is required to access! though practically "a Synaptics touchpad exists" is not very sensitive information that must be hidden from sandboxed apps I guess?)

In D18694#398946, @wulf wrote:

I am objecting to EvdevProbe()-derived part (device type autodetection). It is better to add a KPI to set device type directly rather then via execution of libmagic. We have about 10 evdev drivers in source tree so it's possible to update them all. EvdevProbe()-derived part could be used as (deprecated) fallback option in that case.

Yeah, sounds good. Though I wouldn't say it's only a "deprecated" option — we're not going to require userspace uinput drivers to explicitly identify themselves..

trasz added a subscriber: trasz.Dec 31 2018, 12:24 PM

I'd suggest updating the devd.conf(5) man page as well.

wulf added a comment.Dec 31 2018, 3:50 PM

Though I wouldn't say it's only a "deprecated" option — we're not going to require userspace uinput drivers to explicitly identify themselves..

It is really deprecated. INPUT_PROP_* family is intended to provide information about device type nowadays. E.g. mouses and touchpads should set INPUT_PROP_POINTER property. Touchscreens should set INPUT_PROP_DIRECT and so on. uinput has UI_SET_PROPBIT ioctl for that.

zeising added a subscriber: zeising.Jan 2 2019, 7:29 PM
greg_unrelenting.technology retitled this revision from evdev: add devctl_notify calls for attach/detach events to evdev: add devctl notifications and sysctls with device info.Jan 2 2019, 7:29 PM
greg_unrelenting.technology edited the summary of this revision. (Show Details)
greg_unrelenting.technology updated this revision to Diff 52492.

Update: sysctl edition.

let's quote shortname because uinput devices can put spaces there

imp added a comment.Jan 3 2019, 7:17 PM

This looks generally good to me, but I've not taken the time to verify every bit is correctly serialized / deserialized.

wulf added a comment.Jan 6 2019, 12:03 AM

I am AFK for now and will be for ~2 weeks

wulf added a comment.Jan 21 2019, 12:34 PM

One thing I dislike in this patch is a native/cuse evdev handling inconsistency.
To handle native device creation one should listen for EVDEV devd events while to handle cuse devices CDEV devd events should be processed.
You can not just listen for CDEV devd events as there is a race window between cdev and sysctl creation.
I think this can be fixed with using of single sysctl node. See kern.geom.confxml or kern.geom.conftxt for example.

sys/dev/evdev/evdev.c
43 ↗(On Diff #52493)

style(9) requires #includes to be sorted alphabetically

81 ↗(On Diff #52493)

I am not sure if dev.input is appropriate path to evdev sysctl interface. Most of dev. citizens are device(9) instances rather than cdevs.
IMO kern.evdev. is a better place (that is similar to what geom and cam subsystems do)

385 ↗(On Diff #52493)

Please, place sysctl part in to separate routine

391 ↗(On Diff #52493)

SYSCTL_ADD_STRING is better here (and after). ev_name and other fields are read-only so it is not strictly necessary to protect them with lock.

398 ↗(On Diff #52493)

Please, make sysctl names consistent with ioctl's. Change shortname -> phys, serial -> uniq, flags -> bits and so on.

sys/dev/evdev/evdev_private.h
137 ↗(On Diff #52493)

It`s better to allocate local temporary variables of small size on stack

139 ↗(On Diff #52493)

It is local temp variable too

greg_unrelenting.technology retitled this revision from evdev: add devctl notifications and sysctls with device info to evdev: add sysctls with device info.Feb 2 2019, 10:08 PM
greg_unrelenting.technology edited the summary of this revision. (Show Details)
greg_unrelenting.technology updated this revision to Diff 53550.
greg_unrelenting.technology marked 7 inline comments as done.Feb 2 2019, 10:12 PM
In D18694#403804, @wulf wrote:

To handle native device creation one should listen for EVDEV devd events while to handle cuse devices CDEV devd events should be processed.
You can not just listen for CDEV devd events as there is a race window between cdev and sysctl creation.

I don't actually need the devd notifications for anything (they were a leftover from a stupid way of doing this), so I removed them from the patch. Let's focus on the sysctls, which are actually necessary for running compositors without /dev/input permissions.

added missing EVDEV_UNLOCK

wulf accepted this revision.Feb 10 2019, 7:10 PM

added missing EVDEV_UNLOCK

All data exported through sysctl here is read-only. So there is no need to take any locks. I will handle this.

This revision is now accepted and ready to land.Feb 10 2019, 7:10 PM
wulf added a comment.Feb 15 2019, 10:57 AM

Hi, Greg

I hope you won't mind if I change sysctl name from "input_id" to "id" and disable exposure of optional properties like "uniq" and "phys" if they are not set. Just to be consistent with ioctl interface.

imp accepted this revision.Feb 15 2019, 4:11 PM

This code looks good to me. I take no positions on any name stuff. The one thing I saw is not relevant to this change, so forgive my side comment.

sbin/sysctl/sysctl.c
1004 ↗(On Diff #53741)

this likely shouldn't be amd64 only code, but I'll raise that with andy_t instead.

In D18694#410790, @wulf wrote:

I hope you won't mind if I change sysctl name from "input_id" to "id" and disable exposure of optional properties like "uniq" and "phys" if they are not set. Just to be consistent with ioctl interface.

Yeah, names can be whatever. Always having the same set of properties available would be more convenient though.

This revision was automatically updated to reflect the committed changes.