usb: enhance verification of the class control requests #36694

jfischer-no · 2021-07-02T12:44:33Z

Set data stage length variable to zero as a precaution
so that no trouble happens if control request handler
does not check the request values sufficiently.

Check the control transfer direction in CDC classes, bluetooth, audio, and DFU.

carlescufi · 2021-07-16T11:10:31Z

CC @szymonh

cfriedt · 2021-07-19T08:13:11Z

subsys/usb/usb_device.c

I would suggest changing /** to /* here just so it isn't picked up by doxygen.

cfriedt · 2021-07-19T08:13:47Z

subsys/usb/usb_device.c

I would suggest changing /** to /* here just so it isn't picked up by doxygen

cfriedt · 2021-07-19T08:17:01Z

@jfischer-no - do we have test coverage for most of these changes?

jfischer-no · 2021-07-19T08:41:20Z

@jfischer-no - do we have test coverage for most of these changes?

No, there is no testbench for current USB device stack. I test such changes manually with testusb and USBCV3. For overhauled USB device stack I plan to implement test based on USBIP.

cfriedt · 2021-07-19T08:51:05Z

@jfischer-no - do we have test coverage for most of these changes?

No, there is no testbench for current USB device stack. I test such changes manually with testusb and USBCV3. For overhauled USB device stack I plan to implement test based on USBIP.

Ah, ok. As long as you're testing ;-) Is there someone in particular you would like to review this?

As an alternative to USBIP, there are a few other emulated peripheral drivers that are fairly invaluable to CI / automated testing.

drivers/adc/adc_emul.c
drivers/espi/espi_emul.c
drivers/gpio/gpio_emul.c
drivers/i2c/i2c_emul.c
drivers/spi/spi_emul.c

USB would be great to have on that list.

Kind of surprised it hasn't grown beyond that yet. UART would be an obvious candidate too.

jfischer-no · 2021-07-19T09:08:59Z

@jfischer-no - do we have test coverage for most of these changes?

No, there is no testbench for current USB device stack. I test such changes manually with testusb and USBCV3. For overhauled USB device stack I plan to implement test based on USBIP.

Ah, ok. As long as you're testing ;-) Is there someone in particular you would like to review this?

These are in the review list 😄

As an alternative to USBIP, there are a few other emulated peripheral drivers that are fairly invaluable to CI / automated testing.
drivers/adc/adc_emul.c
drivers/espi/espi_emul.c
drivers/gpio/gpio_emul.c
drivers/i2c/i2c_emul.c
drivers/spi/spi_emul.c
USB would be great to have on that list.

Kind of surprised it hasn't grown beyond that yet. UART would be an obvious candidate too.

It is not that simple. It is not just the driver API (btw, there is simple CI test for USB driver API), it is also the stack that needs to be tested. However, it makes no sense to waste resources on current implementation.

cfriedt · 2021-07-19T14:30:46Z

It is not that simple. It is not just the driver API (btw, there is simple CI test for USB driver API), it is also the stack that needs to be tested. However, it makes no sense to waste resources on current implementation.

The emulated peripherals aren't simple per se - the intent is to use them to achieve 100% test coverage for driver APIs along with the stack.

Would definitely be something worthwhile to consider if we ever do reimplement the USB stack ;-)

cfriedt · 2021-07-22T09:46:21Z

@d3zd3z - can you please take a look at this?

eobalski

NACK just because of missing -ENOTSUP the rest are clarification questions rather than nack

subsys/usb/class/bt_h4.c

subsys/usb/class/msc.c

subsys/usb/class/hid/core.c

subsys/usb/usb_device.c

cfriedt · 2021-07-27T11:01:54Z

@jfischer-no - could you address the comments by @emob-nordic ?

Set data stage length variable to zero as a precaution so that no trouble happens if control request handler does not check the request values sufficiently. Signed-off-by: Johann Fischer <[email protected]>

Rework CDC ACM class control request handling and check the control transfer direction. Signed-off-by: Johann Fischer <[email protected]>

Use internal buffer to store request data and not the data OUT stage buffer. Signed-off-by: Johann Fischer <[email protected]>

Control request handler from CDC ECM implementation does not care at all about returning errors when a request is not supported. Return EINVAL/ENOTSUP if request is incorrect/unsupported. Signed-off-by: Johann Fischer <[email protected]>

Return ENOTSUP if request is unsupported and pass on the error from encapsulated command/response functions. Signed-off-by: Johann Fischer <[email protected]>

Rework mute request handling and add checks for control request direction. Signed-off-by: Johann Fischer <[email protected]>

HCI commands are always directed to controller and should be host-to-device class control requests. Signed-off-by: Johann Fischer <[email protected]>

This class does not handle any vendor request, therefore just return -ENOTSUP on any vendor request. Signed-off-by: Johann Fischer <[email protected]>

Rework class control request handler and add control request direction check. Signed-off-by: Johann Fischer <[email protected]>

The sample only supports device-to-host control requests. Return -ENOTSUP on host-to-device control requests. Signed-off-by: Johann Fischer <[email protected]>

Control pipe in wpanusb is used only for host-to-device requests. Signed-off-by: Johann Fischer <[email protected]>

Check control request direction and split class request handler by request direction. Signed-off-by: Johann Fischer <[email protected]>

Halt endpoint on flash API error or block mismatch. Signed-off-by: Johann Fischer <[email protected]>

Obtain the requested length from wLength and assign the possible length to len argument. Signed-off-by: Johann Fischer <[email protected]>

Cleanup standard device request handler. Pass pointer to setup packet as argument where it is reasonable. Signed-off-by: Johann Fischer <[email protected]>

Cleanup standard endpoint request handler. Signed-off-by: Johann Fischer <[email protected]>

Cleanup standard device request handler. Pass pointer to setup packet as argument where it is reasonable. Signed-off-by: Johann Fischer <[email protected]>

Check request direction in standard device, interface, and endpoint request handlers. Signed-off-by: Johann Fischer <[email protected]>

jfischer-no added the area: USB Universal Serial Bus label Jul 2, 2021

jfischer-no requested review from d3zd3z, nashif, anangl and eobalski July 2, 2021 12:44

jfischer-no force-pushed the pr-usb-fix-cdcacm-request-handling branch from 394e842 to 4401e94 Compare July 2, 2021 14:35

jfischer-no requested review from nvlsianpu, pfalcon, rlubos and tbursztyka as code owners July 6, 2021 09:10

github-actions bot added area: Bluetooth area: Networking area: Samples Samples labels Jul 6, 2021

jfischer-no changed the title ~~usb: cdc_acm: enhance verification of the class control requests~~ usb: enhance verification of the class control requests Jul 6, 2021

jfischer-no force-pushed the pr-usb-fix-cdcacm-request-handling branch from 4b2670d to f2172ef Compare July 6, 2021 09:56

zephyrbot requested a review from mengxianglinx July 6, 2021 13:53

zephyrbot assigned jfischer-no Jul 6, 2021

jfischer-no added this to the v2.7.0 milestone Jul 13, 2021

cfriedt reviewed Jul 19, 2021

View reviewed changes

pfalcon removed their request for review July 19, 2021 14:27

cfriedt approved these changes Jul 19, 2021

View reviewed changes

eobalski suggested changes Jul 22, 2021

View reviewed changes

jfischer-no force-pushed the pr-usb-fix-cdcacm-request-handling branch from 4007f5d to 88521bf Compare August 2, 2021 13:51

jfischer-no requested a review from eobalski August 2, 2021 13:51

eobalski approved these changes Aug 2, 2021

View reviewed changes

jfischer-no added 18 commits August 3, 2021 14:09

usb: set data stage length variable to zero by default

b9c5a17

Set data stage length variable to zero as a precaution so that no trouble happens if control request handler does not check the request values sufficiently. Signed-off-by: Johann Fischer <[email protected]>

usb: cdc_acm: enhance verification of the class control requests

070227f

Rework CDC ACM class control request handling and check the control transfer direction. Signed-off-by: Johann Fischer <[email protected]>

usb: loopback: use class internal buffer to store request data

6c304da

Use internal buffer to store request data and not the data OUT stage buffer. Signed-off-by: Johann Fischer <[email protected]>

usb: rndis: return error on incorrect/unsupported request

67b2616

Return ENOTSUP if request is unsupported and pass on the error from encapsulated command/response functions. Signed-off-by: Johann Fischer <[email protected]>

usb: audio: additionally check control request direction

94b255f

Rework mute request handling and add checks for control request direction. Signed-off-by: Johann Fischer <[email protected]>

usb: bluetooth: additionally check control request direction

54616f6

HCI commands are always directed to controller and should be host-to-device class control requests. Signed-off-by: Johann Fischer <[email protected]>

usb: bt_h4: return -ENOTSUP on vendor requests

daedafa

This class does not handle any vendor request, therefore just return -ENOTSUP on any vendor request. Signed-off-by: Johann Fischer <[email protected]>

usb: msc: additionally check control request direction

8132579

Rework class control request handler and add control request direction check. Signed-off-by: Johann Fischer <[email protected]>

samples: webusb: return -ENOTSUP on host-to-device control requests

cc48574

The sample only supports device-to-host control requests. Return -ENOTSUP on host-to-device control requests. Signed-off-by: Johann Fischer <[email protected]>

samples: wpanusb: return -ENOTSUP on unsupported device-to-host requests

a118cc7

Control pipe in wpanusb is used only for host-to-device requests. Signed-off-by: Johann Fischer <[email protected]>

usb: dfu: check control request direction

9a0c74d

Check control request direction and split class request handler by request direction. Signed-off-by: Johann Fischer <[email protected]>

usb: dfu: halt endpoint on flash API error or block mismatch

ee75e07

Halt endpoint on flash API error or block mismatch. Signed-off-by: Johann Fischer <[email protected]>

usb: hid: obtain the requested length from wLength

d301872

Obtain the requested length from wLength and assign the possible length to len argument. Signed-off-by: Johann Fischer <[email protected]>

usb: cleanup standard device request handler

287b098

Cleanup standard device request handler. Pass pointer to setup packet as argument where it is reasonable. Signed-off-by: Johann Fischer <[email protected]>

usb: cleanup standard endpoing request handler

414e5eb

Cleanup standard endpoint request handler. Signed-off-by: Johann Fischer <[email protected]>

usb: cleanup standard interfave request handler

577f902

Cleanup standard device request handler. Pass pointer to setup packet as argument where it is reasonable. Signed-off-by: Johann Fischer <[email protected]>

usb: check request direction in request handlers

dbfd641

Check request direction in standard device, interface, and endpoint request handlers. Signed-off-by: Johann Fischer <[email protected]>

jfischer-no force-pushed the pr-usb-fix-cdcacm-request-handling branch from 88521bf to dbfd641 Compare August 3, 2021 15:31

nashif merged commit 4b79912 into zephyrproject-rtos:main Aug 3, 2021

jfischer-no deleted the pr-usb-fix-cdcacm-request-handling branch August 4, 2021 07:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

usb: enhance verification of the class control requests #36694

usb: enhance verification of the class control requests #36694

Uh oh!

jfischer-no commented Jul 2, 2021 •

edited

Loading

Uh oh!

carlescufi commented Jul 16, 2021

Uh oh!

cfriedt Jul 19, 2021

Uh oh!

cfriedt Jul 19, 2021

Uh oh!

cfriedt commented Jul 19, 2021

Uh oh!

jfischer-no commented Jul 19, 2021

Uh oh!

cfriedt commented Jul 19, 2021 •

edited

Loading

Uh oh!

jfischer-no commented Jul 19, 2021

Uh oh!

cfriedt commented Jul 19, 2021

Uh oh!

cfriedt commented Jul 22, 2021

Uh oh!

eobalski left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cfriedt commented Jul 27, 2021

Uh oh!

Uh oh!

usb: enhance verification of the class control requests #36694

usb: enhance verification of the class control requests #36694

Uh oh!

Conversation

jfischer-no commented Jul 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlescufi commented Jul 16, 2021

Uh oh!

cfriedt Jul 19, 2021

Choose a reason for hiding this comment

Uh oh!

cfriedt Jul 19, 2021

Choose a reason for hiding this comment

Uh oh!

cfriedt commented Jul 19, 2021

Uh oh!

jfischer-no commented Jul 19, 2021

Uh oh!

cfriedt commented Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jfischer-no commented Jul 19, 2021

Uh oh!

cfriedt commented Jul 19, 2021

Uh oh!

cfriedt commented Jul 22, 2021

Uh oh!

eobalski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cfriedt commented Jul 27, 2021

Uh oh!

Uh oh!

jfischer-no commented Jul 2, 2021 •

edited

Loading

cfriedt commented Jul 19, 2021 •

edited

Loading