On Thu, Dec 11, 2014 at 12:49 AM, David Herrmann [off-list ref] wrote:

Hi

On Wed, Dec 10, 2014 at 6:07 PM, Andy Lutomirski [off-list ref] wrote:

quoted

On Tue, Dec 9, 2014 at 12:46 PM, Andy Lutomirski [off-list ref] wrote:

quoted

On Mon, Nov 3, 2014 at 12:41 PM, Andy Lutomirski [off-list ref] wrote:

quoted

On Mon, Nov 3, 2014 at 12:21 PM, Jiri Kosina [off-list ref] wrote:

quoted

On Mon, 3 Nov 2014, David Herrmann wrote:

quoted

quoted

Agreed, mostly.  My only real concern is that this could be annoying
for the userspace developers who will need to target Linux and HIDAPI
separately.  Admittedly the Linux support will be trivial.

I see. I'll not stop you from using hidraw, I'd just not recommend it.
Especially for security stuff I dislike exposing the whole HID device
writable. But yeah, I guess you got my point.

Alright, I am basically thinking loudly now, but how about we allow HID
drivers that would override default hidraw interface?

I am very well aware of the fact that this could be opening a can of
worms, so we'll have to make it very restrictive. How about, let's say, we
allow HID drivers to provide custom hidraw interface (completely
overriding the one that HID core would normally create) only for cases
such as:

- the intent is to expose only certain parts of a combined device
- the intent is to introduce some level of access control

I.e. still no interference of kernel with data parsing allowed.

Hmm.  Would this be like a filter on hidraw actions?

How would udev distinguish these special hidraw devices from normal
hidraw devices?

Also, for U2F, this could be a little awkward.  There's some crazy
fragmentation stuff to allow a U2F request to be split across HID
requests, and I think a kernel driver would much rather get the
original unfragmented application request.

I started writing a driver for this.  I got enumeration working.  I
assume I should create a "u2f" device class, and then... ?

Where am I supposed to get my character device from?  Do I register my
own chrdev major?  Do I use misc?  Is there some input thing I'm
supposed to use?

Another question:

I'm hitting this in hid_input_report:

    if (down_trylock(&hid->driver_input_lock)) {
        pr_err("HID: trylock failed\n");  // I added this
        return -EBUSY;
    }

This is a problem for u2f: u2f reports are actual protocol messages,
and there isn't a retransmit mechanism.  Losing messages randomly
causes the handshake to fail, and then nothing works.

I *think* that the only way I can hit that failure is during probe (or
if the USB stack somehow completes two transfers at once). This still
makes it quite awkward to start IO from the probe routine.

I could start a work item to take driver_input_lock, release it again,
and then start the handshake, but that sucks.

HID reports are ordered. It's the responsibility of the
transport-layer to not provide multiple reports in parallel. The
reason we have this down_trylock() is to drop packages if no driver is
loaded, yet. I wonder how you can trigger this? Does this happen
during runtime or only driver load? Do you send GET_REPORT requests to
the device?

It happens during driver probe.  I don't set GET_REPORT to my device
-- the protocol consists solely of SET_REPORT from the driver followed
by an asynchronous (but generally reasonably quick) report back from
the device.

During probe, I set INIT, and the device responds by saying "I'm a
device.  I speak this version of the protocol.

Shouldn't that code be more like:

mutex_lock(whatever);
if (driver loaded)
  deliver the report;
mutex_unlock(whatever);

and then the core could hold the lock briefly as well when probing or
in hid_hw_open/hid_hw_close.

(Also, shouldn't hid_hw_open have some kind of reference counting to
avoid interference between hidraw and real drivers?  Or does that
already work correctly somehow?)

--Andy

Thanks
David

-- 
Andy Lutomirski
AMA Capital Management, LLC