Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

usb: device_next: usbd_hid: Fix size in HID report get #79001

Merged
merged 1 commit into from
Sep 30, 2024
Merged

usb: device_next: usbd_hid: Fix size in HID report get #79001

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions subsys/usb/device_next/class/usbd_hid.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@
const uint8_t id = HID_GET_REPORT_ID(setup->wValue);
struct hid_device_data *const ddata = dev->data;
const struct hid_device_ops *ops = ddata->ops;
const size_t size = net_buf_tailroom(buf);
const size_t size = setup->wLength;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if using setup->wLength directly is appropriate, a potential alternative would be MIN(setup->wLength, net_buf_tailroom(buf)). Host can submit any wLength value up to 65535 regardless the actual buffer size. Currently the stack will fail if it cannot allocate such large request even if the actual payloads are much smaller.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can use MIN(setup->wLength, net_buf_tailroom(buf) here. Still, I wonder if the condition should not be validated somewhere in lower layers (so that USB class implementation could already assume that setup->wLength is proper.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more of a wondering how this issue should be solved in general. This sort of mechanism currently results in USB2CV MSC failure with default build settings because the testcase is requesting string descriptors length with 4096 wLength whereas the strings are significantly shorter than that. While the strings would definitely fit inside the much smaller buffer, they are not even given a chance because default UDC buf pool is failing to allocate 4096 bytes long buffer.

int ret = 0;

switch (type) {
Expand All @@ -257,8 +257,9 @@
}

if (ret > 0) {
__ASSERT(ret <= size, "Buffer overflow in the HID driver");
net_buf_add(buf, MIN(size, ret));
__ASSERT(ret <= net_buf_tailroom(buf),
"Buffer overflow in the HID driver");
net_buf_add(buf, MIN(net_buf_tailroom(buf), ret));

Check notice on line 262 in subsys/usb/device_next/class/usbd_hid.c

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

You may want to run clang-format on this change 

subsys/usb/device_next/class/usbd_hid.c:262 
-		__ASSERT(ret <= net_buf_tailroom(buf),
-			 "Buffer overflow in the HID driver");
+		__ASSERT(ret <= net_buf_tailroom(buf), "Buffer overflow in the HID driver");
} else {
errno = ret ? ret : -ENOTSUP;
}
Expand Down
Loading