usb(4): Separate the fast path and the slow path to avoid races and use-after-free for the USB FS interface

[• hselasky]

Summary

Bad behaving user-space USB applicatoins may crash the kernel by issuing
USB FS related ioctl(2)'s out of their expected order. By default
the USB FS ioctl(2) interface is only available to the
administrator, root, and driver applications like webcamd(8) needs
to be hijacked in order for this to happen.

The issue is that the fast-path code does not always see updates made
by the slow-path code, and may then work on freed memory.

This is easily fixed by using an EPOCH(9) type of synchronization
mechanism. A SX(9) lock will be used as a substitute for EPOCH(9),
due to the need for sleepability. In addition most calls going into
the fast-path originate from a single user-space process and the
need for multi-thread performance is not present.

Reported by: C Turt <[e-mail]>
MFC after: 1 week
Sponsored by: NVIDIA Networking

[markj]

I'll be away this weekend and won't have time to review until Monday at the earliest.

I'd like to use a sleepable EPOCH in the fast path, but we don't have the currently - any other ideas?

Use a pair of sx locks, and require both of them for the slow path, but only require a single shared lock for the fast path? Is concurrency a major concern, given that the lock is scoped to a specific USB device?

[• hselasky]

The problem is the fast path does copy in / out and need the sleepable property too. Do the shared locks you have in mind support that?

--HPS

[markj]

In D39373#896157, @hselasky wrote:

The problem is the fast path does copy in / out and need the sleepable property too. Do the shared locks you have in mind support that?

--HPS

sx locks are sleepable, yes. I just mean the "shared" mode of sx locks. Or copy in to a temporary buffer without holding any locks, or use a mechanism to sysctl_wire_old_buffer() before acquiring any locks, so that copying can be done without faulting, and thus without sleeping.

[• hselasky]

I'm going to test this a bit and then head for a push.

If you see any problems, let me know.

I've taken the SX-lock approach as described by Mark Johnston.

Hopefully USB will function as before!

[• hselasky]

Test: OK

[• hselasky]

Can someone test / accept this patch?

[markj]

driver applications like webcamd(8) needs to be hijacked in order for this to happen.

Any privileged application can open ugen devices.

The patch looks like it fixes the reported problem. It is hard to review since the commit also moves ioctl handlers into their own functions, so it's not easy to see what actually changed.

Please be sure to include "Reported by: C Turt <[e-mail]>" in the commit message.

sys/dev/usb/usb_generic.c

2444–2445

Why not have this check in ugen_fs_init(), which also verifies that ep_index_max != 0?

[emaste]

the commit also moves ioctl handlers into their own functions

Could the change be split into two parts, first move code to usb_fs_int and usb_fs_open, then the functional part of the change?

[• hselasky]

Implement suggestions from @markj .

[• hselasky]

sys/dev/usb/usb_generic.c

2444–2445

Good point. Done!

[• hselasky]

@emaste: Yes, I can split the diff into two parts like you suggest.

[• hselasky]

@emaste : Patch is now split. It won't apply to 14-current, but should be easier to review.

Please accept when you're done and I'll push it!

[markj]

Thank you, this looks ok to me.

sys/dev/usb/usb_generic.c

1748

It looks like the fastpath lock isn't needed here, since priv_mtx is held all the way through. It doesn't hurt, though.

[• hselasky]

sys/dev/usb/usb_generic.c

1748

You are right. I'll strip that before commit.