usb: gadget: add raw-gadget interface

USB Raw Gadget is a kernel module that provides a userspace interface for the USB Gadget subsystem. Essentially it allows to emulate USB devices from userspace. Enabled with CONFIG_USB_RAW_GADGET. Raw Gadget is currently a strictly debugging feature and shouldn't be used in production.

Raw Gadget is similar to GadgetFS, but provides a more low-level and direct access to the USB Gadget layer for the userspace. The key differences are:

1. Every USB request is passed to the userspace to get a response, while GadgetFS responds to some USB requests internally based on the provided descriptors. However note, that the UDC driver might respond to some requests on its own and never forward them to the Gadget layer.

2. GadgetFS performs some sanity checks on the provided USB descriptors, while Raw Gadget allows you to provide arbitrary data as responses to USB requests.

3. Raw Gadget provides a way to select a UDC device/driver to bind to, while GadgetFS currently binds to the first available UDC.

4. Raw Gadget uses predictable endpoint names (handles) across different UDCs (as long as UDCs have enough endpoints of each required transfer type).

5. Raw Gadget has ioctl-based interface instead of a filesystem-based one.

Signed-off-by Andrey Konovalov <[email protected]>

Quoted 26 lines · Andrey Konovalov

USB Raw Gadget is a kernel module that provides a userspace interface for the USB Gadget subsystem. Essentially it allows to emulate USB devices from userspace. Enabled with CONFIG_USB_RAW_GADGET. Raw Gadget is currently a strictly debugging feature and shouldn't be used in production.

Raw Gadget is similar to GadgetFS, but provides a more low-level and direct access to the USB Gadget layer for the userspace. The key differences are:

1. Every USB request is passed to the userspace to get a response, while GadgetFS responds to some USB requests internally based on the provided descriptors. However note, that the UDC driver might respond to some requests on its own and never forward them to the Gadget layer.

2. GadgetFS performs some sanity checks on the provided USB descriptors, while Raw Gadget allows you to provide arbitrary data as responses to USB requests.

3. Raw Gadget provides a way to select a UDC device/driver to bind to, while GadgetFS currently binds to the first available UDC.

4. Raw Gadget uses predictable endpoint names (handles) across different UDCs (as long as UDCs have enough endpoints of each required transfer type).

Quoted 3 lines

5. Raw Gadget has ioctl-based interface instead of a filesystem-based one.

Signed-off-by Andrey Konovalov <[email protected]>

This looks good to me, with the one minor problem below that you can fix with a follow-on patch.

This should probably go through Felipe's tree
though, so I'll wait for him to review the gadget side and then queue it up in his tree:

Quoted 10 lines

include/uapi/linux/usb/raw_gadget.h new file +10 -0

+/* The type of event fetched with the USB_RAW_IOCTL_EVENT_FETCH ioctl. */
+enum usb_raw_event_type {
+     USB_RAW_EVENT_INVALID,
+
+     /* This event is queued when the driver has bound to a UDC. */
+     USB_RAW_EVENT_CONNECT,
+
+     /* This event is queued when a new control request arrived to ep0. */
+     USB_RAW_EVENT_CONTROL,
+

Review

Quoted 2 lines

(quoted diff) +2 -0

+     /* The list might grow in the future. */
+};

You have to manually specify the enum values in the .h file for all entries in order to assure that both userspace and the kernel will be in sync with the same values. I think that's documented in the "how to write an ioctl interface" document that is somewhere in Documentation/...

I've also found an issue, but I'm not sure if that is the bug in Raw Gadget, or in the gadget layer (in the former case I'll add this fix
to v5 as well). What I believe I'm seeing is __fput()->usb_gadget_unregister_driver()->usb_gadget_remove_driver()->gadget_unbind() racing with dummy_timer()->gadget_setup(). In my case it results in gadget_unbind() doing set_gadget_data(gadget, NULL), and then gadget_setup() dereferencing get_gadget_data(gadget).

Alan, does it look possible for those two functions to race? Should this be prevented by the gadget layer, or should I use some kind of locking in my gadget driver to prevent this?

Quoted 11 lines · Andrey Konovalov

I've also found an issue, but I'm not sure if that is the bug in Raw Gadget, or in the gadget layer (in the former case I'll add this fix
to v5 as well). What I believe I'm seeing is __fput()->usb_gadget_unregister_driver()->usb_gadget_remove_driver()->gadget_unbind() racing with dummy_timer()->gadget_setup(). In my case it results in gadget_unbind() doing set_gadget_data(gadget, NULL), and then gadget_setup() dereferencing get_gadget_data(gadget).

Alan, does it look possible for those two functions to race? Should this be prevented by the gadget layer, or should I use some kind of locking in my gadget driver to prevent this?

In your situation this race shouldn't happen, because before udc->driver->unbind() is invoked we call usb_gadget_disconnect(). If that routine succeeds -- which it always does under dummy-hcd -- then there can't be any more setup callbacks, because find_endpoint() will always return NULL (the is_active() test fails; see the various set_link_state* routines). So I don't see how you could have ended up with the race you describe.

However, a real UDC might not be able to perform a disconnect under software control. In that case usb_gadget_disconnect() would not change the pullup state, and there would be a real possibility of a setup callback racing with an unbind callback. This seems like a genuine problem and I can't think of a solution offhand.

What we would need is a way to tell the UDC driver to stop invoking gadget callbacks, _before_ the UDC driver's stop callback gets called. Maybe this should be merged into the pullup callback somehow.

Alan Stern

Quoted 15 lines · Alan Stern

Quoted 11 lines · Andrey Konovalov

I've also found an issue, but I'm not sure if that is the bug in Raw Gadget, or in the gadget layer (in the former case I'll add this fix
to v5 as well). What I believe I'm seeing is __fput()->usb_gadget_unregister_driver()->usb_gadget_remove_driver()->gadget_unbind() racing with dummy_timer()->gadget_setup(). In my case it results in gadget_unbind() doing set_gadget_data(gadget, NULL), and then gadget_setup() dereferencing get_gadget_data(gadget).

Alan, does it look possible for those two functions to race? Should this be prevented by the gadget layer, or should I use some kind of locking in my gadget driver to prevent this?

Quoted 7 lines

In your situation this race shouldn't happen, because before udc->driver->unbind() is invoked we call usb_gadget_disconnect(). If that routine succeeds -- which it always does under dummy-hcd -- then there can't be any more setup callbacks, because find_endpoint() will always return NULL (the is_active() test fails; see the various set_link_state* routines). So I don't see how you could have ended up with the race you describe.

I've managed to reproduce the race by adding an mdelay() into the beginning of the setup() callback. AFAIU what happens is setup() gets called (and waits on the mdelay()), then unbind() comes in and does set_gadget_data(NULL), and then setup() proceeds, gets NULL through get_gadget_data() and crashes on null-ptr-deref. I've got the same crash a few times after many days of fuzzing, so I assume it can happen without the mdelay() as well.

Quoted 9 lines

However, a real UDC might not be able to perform a disconnect under software control. In that case usb_gadget_disconnect() would not change the pullup state, and there would be a real possibility of a setup callback racing with an unbind callback. This seems like a genuine problem and I can't think of a solution offhand.

What we would need is a way to tell the UDC driver to stop invoking gadget callbacks, _before_ the UDC driver's stop callback gets called. Maybe this should be merged into the pullup callback somehow.

Quoted 39 lines · Andrey Konovalov

Quoted 15 lines · Alan Stern

Quoted 11 lines · Andrey Konovalov

I've also found an issue, but I'm not sure if that is the bug in Raw Gadget, or in the gadget layer (in the former case I'll add this fix
to v5 as well). What I believe I'm seeing is __fput()->usb_gadget_unregister_driver()->usb_gadget_remove_driver()->gadget_unbind() racing with dummy_timer()->gadget_setup(). In my case it results in gadget_unbind() doing set_gadget_data(gadget, NULL), and then gadget_setup() dereferencing get_gadget_data(gadget).

Alan, does it look possible for those two functions to race? Should this be prevented by the gadget layer, or should I use some kind of locking in my gadget driver to prevent this?

Quoted 7 lines

In your situation this race shouldn't happen, because before udc->driver->unbind() is invoked we call usb_gadget_disconnect(). If that routine succeeds -- which it always does under dummy-hcd -- then there can't be any more setup callbacks, because find_endpoint() will always return NULL (the is_active() test fails; see the various set_link_state* routines). So I don't see how you could have ended up with the race you describe.

I've managed to reproduce the race by adding an mdelay() into the beginning of the setup() callback. AFAIU what happens is setup() gets called (and waits on the mdelay()), then unbind() comes in and does set_gadget_data(NULL), and then setup() proceeds, gets NULL through get_gadget_data() and crashes on null-ptr-deref. I've got the same crash a few times after many days of fuzzing, so I assume it can happen without the mdelay() as well.

Quoted 6 lines

However, a real UDC might not be able to perform a disconnect under software control. In that case usb_gadget_disconnect() would not change the pullup state, and there would be a real possibility of a setup callback racing with an unbind callback. This seems like a genuine problem and I can't think of a solution offhand.

Quoted 3 lines

Quoted 3 lines

What we would need is a way to tell the UDC driver to stop invoking gadget callbacks, _before_ the UDC driver's stop callback gets called. Maybe this should be merged into the pullup callback somehow.

Perhaps for the dummy driver we need to wait for setup() to finish if it's being executed and then stop the dummy timer in dummy_pullup()?

This patchset (currently a single patch) adds a new userspace interface for the USB Gadget subsystem called USB Raw Gadget (I don't mind changing the name to something else if there are better ideas). This is what currently being used to enable coverage-buided USB fuzzing with syzkaller:

https://github.com/google/syzkaller/blob/master/docs/linux/external_fuzzing_usb.md

Initially I was using GadgetFS (together with the Dummy HCD/UDC module) to perform emulation of USB devices for fuzzing, but later switched to a custom written interface. The incentive to implement a different interface was to provide a somewhat raw and direct access to the USB Gadget layer for the userspace, where every USB request is passed to the userspace to get a response. See documentation for the list of differences between Raw Gadget and GadgetFS.

This patchset has been pushed to the public Linux kernel Gerrit instance:

https://linux-review.googlesource.com/c/linux/kernel/git/torvalds/linux/+/2144

Changes v3 -> v4:
- Print debug message when maxpacket check fails.
- Use module_misc_device() instead of module_init/exit().
- Reuse DRIVER_NAME macro in raw_device struct definition.
- Don't print WARNING in raw_release().
- Add comment that explains locking into raw_event_queue_fetch().
- Print a WARNING when event queue size is exceeded.
- Rename raw.c to raw_gadget.c.
- Mention module name in Kconfig.
- Reworked logging to use dev_err/dbg() instead of pr_err/debug().

Changes v2 -> v3:
- Updated device path in documentation.
- Changed usb_raw_init struct layout to make it the same for 32 bit compat mode.
- Added compat_ioctl to raw_fops.
- Changed raw_ioctl_init() to return EINVAL for invalid USB speeds, except for USB_SPEED_UNKNOWN, which defaults to USB_SPEED_HIGH.
- Reject endpoints with maxpacket = 0 in raw_ioctl_ep_enable().

Changes v1 -> v2:
- Moved raw.c to legacy/.
- Changed uapi header to use __u* types.
- Switched from debugfs entry to a misc device.
- Changed raw_dev from refcount to kref.
- Moved UDC_NAME_LENGTH_MAX to uapi headers.
- Used usb_endpoint_type() and usb_endpoint_dir_in/out() functions instead of open coding them.
- Added "WITH Linux-syscall-note" to SPDX id in the uapi header.
- Removed pr_err() if case dev_new() fails.
- Reduced the number of debugging messages.

Andrey Konovalov (1):
usb: gadget: add raw-gadget interface

 Documentation/usb/index.rst            |    1 +
 Documentation/usb/raw-gadget.rst       |   59 ++
 drivers/usb/gadget/legacy/Kconfig      |   11 +
 drivers/usb/gadget/legacy/Makefile     |    1 +
 drivers/usb/gadget/legacy/raw_gadget.c | 1071 ++++++++++++++++++++++++
 include/uapi/linux/usb/raw_gadget.h    |  167 ++++
 6 files changed, 1310 insertions(+)
 create mode 100644 Documentation/usb/raw-gadget.rst
 create mode 100644 drivers/usb/gadget/legacy/raw_gadget.c
 create mode 100644 include/uapi/linux/usb/raw_gadget.h

--
2.24.1.735.g03f4e72817-goog