Page MenuHomeFreeBSD

evdev: add sysctls with device info
ClosedPublic

Authored by val_packett.cool on Dec 30 2018, 9:37 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 22, 11:45 PM
Unknown Object (File)
Fri, Mar 22, 11:45 PM
Unknown Object (File)
Fri, Mar 22, 11:45 PM
Unknown Object (File)
Fri, Mar 22, 11:45 PM
Unknown Object (File)
Fri, Mar 22, 11:45 PM
Unknown Object (File)
Fri, Mar 22, 11:45 PM
Unknown Object (File)
Fri, Mar 22, 11:45 PM
Unknown Object (File)
Sat, Mar 9, 8:29 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 - subversion
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

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..

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

In D18694#398967, @greg_unrelenting.technology wrote:

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.

val_packett.cool retitled this revision from evdev: add devctl_notify calls for attach/detach events to evdev: add devctl notifications and sysctls with device info.
val_packett.cool edited the summary of this revision. (Show Details)

Update: sysctl edition.

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

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

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

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

style(9) requires #includes to be sorted alphabetically

81

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

Please, place sysctl part in to separate routine

391

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

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

val_packett.cool retitled this revision from evdev: add devctl notifications and sysctls with device info to evdev: add sysctls with device info.
val_packett.cool edited the summary of this revision. (Show Details)
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.

In D18694#409332, @greg_unrelenting.technology wrote:

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

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.

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.