On Wed, Dec 18, 2019 at 08:26:57PM +0100, Andrey Konovalov wrote:
> 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]>
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:
> +/* 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,
> +
> + /* 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/...
thanks,
greg k-h
On Sat, Jan 11, 2020 at 9:02 PM Greg Kroah-Hartman
<[email protected]> wrote:
>
> On Wed, Dec 18, 2019 at 08:26:57PM +0100, Andrey Konovalov wrote:
> > 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]>
>
> This looks good to me, with the one minor problem below that you can fix
> with a follow-on patch.
Great, thanks! I'd prefer to send out v5 to keep this a single patch
if that's OK.
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?
> 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:
OK, looking forward to Felipe's review.
> > +/* 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,
> > +
> > + /* 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/...
Will fix in v5, thanks!
On Mon, 13 Jan 2020, Andrey Konovalov wrote:
> 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
On Mon, Jan 13, 2020 at 5:50 PM Alan Stern <[email protected]> wrote:
>
> On Mon, 13 Jan 2020, Andrey Konovalov wrote:
>
> > 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.
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.
> 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
>
On Mon, Jan 13, 2020 at 6:34 PM Andrey Konovalov <[email protected]> wrote:
>
> On Mon, Jan 13, 2020 at 5:50 PM Alan Stern <[email protected]> wrote:
> >
> > On Mon, 13 Jan 2020, Andrey Konovalov wrote:
> >
> > > 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.
>
> 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.
>
> > 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.
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()?
On Mon, 13 Jan 2020, Andrey Konovalov wrote:
> On Mon, Jan 13, 2020 at 6:34 PM Andrey Konovalov <[email protected]> wrote:
> >
> > On Mon, Jan 13, 2020 at 5:50 PM Alan Stern <[email protected]> wrote:
> > >
> > > On Mon, 13 Jan 2020, Andrey Konovalov wrote:
> > >
> > > > 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.
> >
> > 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.
> >
> > > 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.
>
> 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()?
Yes, we need to wait for a setup callback to finish. But dummy_timer
should not be stopped; otherwise URBs that have already been submitted
would never be given back.
The big problem is that usb_gadget_disconnect() can be called in
interrupt context. In general, a UDC driver will need to call
synchronize_irq() to make sure there aren't any callbacks still
running, and that can be done only in process context. dummy-hcd is
slightly different since it doesn't manage actual hardware; it calls
usleep_range() instead of synchronize_irq() -- but that also requires
process context.
We could change the gadget API to require that usb_gadget_disconnect()
and related routines always be called in process context. I don't know
if that's such a good idea though. A gadget driver might want to
disconnect from within its setup handler or a completion routine, for
example.
A better approach would be to add a new synchronize_callbacks()
pointer in the usb_gadget_ops structure. But to work properly it
would have to be mandatory for every UDC driver, and that's not so easy
to accomplish.
Suggestions?
Alan Stern