From 217d59ce685eb3f5fa64b1b299e10b5f0a4d57d5 Mon Sep 17 00:00:00 2001 From: Christian Helmuth Date: Thu, 23 Mar 2023 16:31:08 +0100 Subject: [PATCH] usb: use buffer type in C API Also, some reasonable sanity checks of client-passed parameters were added and for-int loops replaced by for-range loops where applicable. Issue #4749 --- repos/dde_linux/src/lib/lx_emul/usb.c | 105 +++++++++++++++----------- repos/os/include/genode_c_api/usb.h | 13 +++- repos/os/src/lib/genode_c_api/usb.cc | 95 +++++++++++++---------- 3 files changed, 125 insertions(+), 88 deletions(-) diff --git a/repos/dde_linux/src/lib/lx_emul/usb.c b/repos/dde_linux/src/lib/lx_emul/usb.c index 8b38582fac..98e7c402a4 100644 --- a/repos/dde_linux/src/lib/lx_emul/usb.c +++ b/repos/dde_linux/src/lib/lx_emul/usb.c @@ -334,7 +334,7 @@ struct genode_usb_rpc_callbacks lx_emul_usb_rpc_callbacks = { static genode_usb_request_ret_t -handle_return_code(struct genode_usb_request_urb req, void * data) +handle_return_code(struct genode_usb_request_urb req, struct genode_usb_buffer payload, void * data) { return (genode_usb_request_ret_t)data; }; @@ -406,14 +406,17 @@ static void handle_string_request(struct genode_usb_request_string * req, genode_usb_session_handle_t session, genode_usb_request_handle_t request, - void * buf, - unsigned long size, + struct genode_usb_buffer payload, void * data) { struct usb_device * udev = (struct usb_device *) data; genode_usb_request_ret_t ret = UNKNOWN_ERROR; + int length = 0; - int length = usb_string_utf16(udev, req->index, buf, size); + if (!payload.size || !payload.addr) + return; + + length = usb_string_utf16(udev, req->index, payload.addr, payload.size); if (length < 0) { printk("Could not read string descriptor index: %u\n", req->index); req->length = 0; @@ -503,6 +506,7 @@ struct usb_urb_context static genode_usb_request_ret_t handle_transfer_response(struct genode_usb_request_urb req, + struct genode_usb_buffer payload, void * data) { struct usb_urb_context * context = (struct usb_urb_context *) data; @@ -513,35 +517,37 @@ handle_transfer_response(struct genode_usb_request_urb req, genode_usb_get_request_transfer(&req); int i; - if (urb->status == 0) { + /* handle failure first */ + if (urb->status) { if (ctrl) - ctrl->actual_size = urb->actual_length; - if (transfer) { - transfer->actual_size = urb->actual_length; + ctrl->actual_size = 0; - if (usb_pipein(urb->pipe)) - for (i = 0; i < urb->number_of_packets; i++) - transfer->actual_packet_size[i] = - urb->iso_frame_desc[i].actual_length; - } - return NO_ERROR; - } + if (context->timer_state == TIMER_TRIGGERED) + return TIMEOUT_ERROR; - if (ctrl) - ctrl->actual_size = 0; - - if (context->timer_state == TIMER_TRIGGERED) - return TIMEOUT_ERROR; - - switch (urb->status) { + switch (urb->status) { case -ENOENT: return INTERFACE_OR_ENDPOINT_ERROR; case -ENODEV: return NO_DEVICE_ERROR; case -ESHUTDOWN: return NO_DEVICE_ERROR; case -EPROTO: return PROTOCOL_ERROR; case -EILSEQ: return PROTOCOL_ERROR; case -EPIPE: return STALL_ERROR; - }; - return UNKNOWN_ERROR; + }; + return UNKNOWN_ERROR; + } + + if (ctrl) + ctrl->actual_size = urb->actual_length; + if (transfer) { + transfer->actual_size = urb->actual_length; + + if (usb_pipeisoc(urb->pipe) && usb_pipein(urb->pipe)) { + for (i = 0; i < urb->number_of_packets; i++) + transfer->actual_packet_size[i] = + urb->iso_frame_desc[i].actual_length; + } + } + return NO_ERROR; } @@ -590,8 +596,7 @@ static void urb_timeout(struct timer_list *t) static int fill_ctrl_urb(struct usb_device * udev, struct genode_usb_request_control * req, void * handle, - void * buf, - unsigned long size, + struct genode_usb_buffer buf, int read, struct urb ** urb) { @@ -599,6 +604,9 @@ static int fill_ctrl_urb(struct usb_device * udev, struct usb_ctrlrequest * ucr = kmalloc(sizeof(struct usb_ctrlrequest), GFP_NOIO); + if (buf.size && !buf.addr) + return PACKET_INVALID_ERROR; + *urb = usb_alloc_urb(0, GFP_KERNEL); if (!ucr || !*urb) { @@ -611,9 +619,9 @@ static int fill_ctrl_urb(struct usb_device * udev, ucr->bRequest = req->request; ucr->wValue = cpu_to_le16(req->value); ucr->wIndex = cpu_to_le16(req->index); - ucr->wLength = cpu_to_le16(size); + ucr->wLength = cpu_to_le16(buf.size); - usb_fill_control_urb(*urb, udev, pipe, (unsigned char*)ucr, buf, size, + usb_fill_control_urb(*urb, udev, pipe, (unsigned char*)ucr, buf.addr, buf.size, async_complete, handle); return 0; } @@ -622,19 +630,21 @@ static int fill_ctrl_urb(struct usb_device * udev, static int fill_bulk_urb(struct usb_device * udev, struct genode_usb_request_transfer * req, void * handle, - void * buf, - unsigned long size, + struct genode_usb_buffer buf, int read, struct urb ** urb) { int pipe = (read) ? usb_rcvbulkpipe(udev, req->ep) : usb_sndbulkpipe(udev, req->ep); + if (!buf.addr) + return PACKET_INVALID_ERROR; + *urb = usb_alloc_urb(0, GFP_KERNEL); if (!*urb) return -ENOMEM; - usb_fill_bulk_urb(*urb, udev, pipe, buf, size, async_complete, handle); + usb_fill_bulk_urb(*urb, udev, pipe, buf.addr, buf.size, async_complete, handle); return 0; } @@ -642,8 +652,7 @@ static int fill_bulk_urb(struct usb_device * udev, static int fill_irq_urb(struct usb_device * udev, struct genode_usb_request_transfer * req, void * handle, - void * buf, - unsigned long size, + struct genode_usb_buffer buf, int read, struct urb ** urb) { @@ -651,6 +660,9 @@ static int fill_irq_urb(struct usb_device * udev, int pipe = (read) ? usb_rcvintpipe(udev, req->ep) : usb_sndintpipe(udev, req->ep); + if (buf.size && !buf.addr) + return PACKET_INVALID_ERROR; + *urb = usb_alloc_urb(0, GFP_KERNEL); if (!*urb) return -ENOMEM; @@ -667,7 +679,7 @@ static int fill_irq_urb(struct usb_device * udev, } else polling_interval = req->polling_interval; - usb_fill_int_urb(*urb, udev, pipe, buf, size, + usb_fill_int_urb(*urb, udev, pipe, buf.addr, buf.size, async_complete, handle, polling_interval); return 0; } @@ -676,8 +688,7 @@ static int fill_irq_urb(struct usb_device * udev, static int fill_isoc_urb(struct usb_device * udev, struct genode_usb_request_transfer * req, void * handle, - void * buf, - unsigned long size, + struct genode_usb_buffer buf, int read, struct urb ** urb) { @@ -688,6 +699,10 @@ static int fill_isoc_urb(struct usb_device * udev, struct usb_host_endpoint * ep = req->ep & USB_DIR_IN ? udev->ep_in[req->ep & 0xf] : udev->ep_out[req->ep & 0xf]; + + + if (!buf.addr) + return PACKET_INVALID_ERROR; if (!ep) return -ENOENT; @@ -699,8 +714,8 @@ static int fill_isoc_urb(struct usb_device * udev, (*urb)->pipe = pipe; (*urb)->start_frame = -1; (*urb)->stream_id = 0; - (*urb)->transfer_buffer = buf; - (*urb)->transfer_buffer_length = size; + (*urb)->transfer_buffer = buf.addr; + (*urb)->transfer_buffer_length = buf.size; (*urb)->number_of_packets = req->number_of_packets; (*urb)->interval = 1 << min(15, ep->desc.bInterval - 1); (*urb)->context = handle; @@ -721,7 +736,7 @@ static void handle_urb_request(struct genode_usb_request_urb req, genode_usb_session_handle_t session_handle, genode_usb_request_handle_t request_handle, - void * buf, unsigned long size, void * data) + struct genode_usb_buffer payload, void * data) { struct usb_device * udev = (struct usb_device *) data; struct usb_iface_urbs * urbs = dev_get_drvdata(&udev->dev); @@ -747,21 +762,21 @@ handle_urb_request(struct genode_usb_request_urb req, switch (req.type) { case CTRL: - err = fill_ctrl_urb(udev, ctrl, context, buf, size, + err = fill_ctrl_urb(udev, ctrl, context, payload, (ctrl->request_type & 0x80), &urb); break; case BULK: - err = fill_bulk_urb(udev, transfer, context, buf, size, read, &urb); + err = fill_bulk_urb(udev, transfer, context, payload, read, &urb); break; case IRQ: - err = fill_irq_urb(udev, transfer, context, buf, size, read, &urb); + err = fill_irq_urb(udev, transfer, context, payload, read, &urb); break; case ISOC: - err = fill_isoc_urb(udev, transfer, context, buf, size, read, &urb); + err = fill_isoc_urb(udev, transfer, context, payload, read, &urb); break; default: printk("Unknown USB transfer request!\n"); - goto error; + err = PACKET_INVALID_ERROR; }; if (err) @@ -792,6 +807,8 @@ handle_urb_request(struct genode_usb_request_urb req, case -ESHUTDOWN: ret = NO_DEVICE_ERROR; break; case -ENOSPC: ret = STALL_ERROR; break; case -ENOMEM: ret = MEMORY_ERROR; break; + + case PACKET_INVALID_ERROR: ret = PACKET_INVALID_ERROR; break; } error: genode_usb_ack_request(context->session, context->request, diff --git a/repos/os/include/genode_c_api/usb.h b/repos/os/include/genode_c_api/usb.h index e7335c3598..8c46545a47 100644 --- a/repos/os/include/genode_c_api/usb.h +++ b/repos/os/include/genode_c_api/usb.h @@ -168,6 +168,12 @@ struct genode_usb_request_urb void * req; }; +struct genode_usb_buffer +{ + void * addr; + unsigned long size; +}; + static inline struct genode_usb_request_control * genode_usb_get_request_control(struct genode_usb_request_urb * urb) { @@ -197,16 +203,14 @@ typedef void (*genode_usb_req_urb_t) (struct genode_usb_request_urb req, genode_usb_session_handle_t session_handle, genode_usb_request_handle_t request_handle, - void * payload, - unsigned long payload_size, + struct genode_usb_buffer payload, void * opaque_data); typedef void (*genode_usb_req_string_t) (struct genode_usb_request_string * req, genode_usb_session_handle_t session_handle, genode_usb_request_handle_t request_handle, - void * payload, - unsigned long payload_size, + struct genode_usb_buffer payload, void * opaque_data); typedef void (*genode_usb_req_altsetting_t) @@ -229,6 +233,7 @@ typedef void (*genode_usb_req_flush_t) typedef genode_usb_request_ret_t (*genode_usb_response_t) (struct genode_usb_request_urb req, + struct genode_usb_buffer payload, void * opaque_data); struct genode_usb_request_callbacks { diff --git a/repos/os/src/lib/genode_c_api/usb.cc b/repos/os/src/lib/genode_c_api/usb.cc index c0234eef21..04b396b5d1 100644 --- a/repos/os/src/lib/genode_c_api/usb.cc +++ b/repos/os/src/lib/genode_c_api/usb.cc @@ -87,6 +87,8 @@ class genode_usb_session : public Usb::Session_rpc_object unsigned _packets_in_fly(); void _ack(int err, Usb::Packet_descriptor p); + genode_usb_buffer _buffer_of_packet(Usb::Packet_descriptor p); + /* * Non_copyable */ @@ -188,9 +190,9 @@ class Root : public Root_component template void _for_each_device(FUNC const & fn) { - for (unsigned idx = 0; idx < MAX_DEVICES; idx++) - if (_devices[idx].constructed()) - fn(*_devices[idx]); + for (auto &device : _devices) + if (device.constructed()) + fn(*device); } template @@ -278,11 +280,11 @@ void genode_usb_session::notify() void genode_usb_session::flush_packet_stream() { /* ack packets in flight */ - for (unsigned idx = 0; idx < MAX_PACKETS_IN_FLY; idx++) { - if (!packets[idx].constructed()) + for (auto &p : packets) { + if (!p.constructed()) continue; - _ack(Usb::Packet_descriptor::NO_DEVICE_ERROR, *packets[idx]); - packets[idx].destruct(); + _ack(Usb::Packet_descriptor::NO_DEVICE_ERROR, *p); + p.destruct(); } /* ack all packets in request stream */ @@ -415,8 +417,8 @@ void genode_usb_session::release_interface(unsigned iface) unsigned genode_usb_session::_packets_in_fly() { unsigned ret = 0; - for (unsigned idx = 0; idx < MAX_PACKETS_IN_FLY; idx++) - if (packets[idx].constructed()) ret++; + for (auto const &p : packets) + if (p.constructed()) ret++; return ret; } @@ -435,16 +437,31 @@ void genode_usb_session::_ack(int err, Usb::Packet_descriptor p) } +genode_usb_buffer genode_usb_session::_buffer_of_packet(Usb::Packet_descriptor p) +{ + void * addr = nullptr; + void * packet_content = sink()->packet_content(p); + if (packet_content) { + addr_t offset = (addr_t)packet_content - (addr_t)sink()->ds_local_base(); + + addr = (void*)(genode_shared_dataspace_local_address(_ds) + offset); + } + + return { addr, addr ? p.size() : 0 }; +} + + bool genode_usb_session::request(genode_usb_request_callbacks & req, void * data) { using Packet_descriptor = Usb::Packet_descriptor; - genode_usb_request_handle_t idx; + genode_usb_request_handle_t idx = 0; /* find free packet slot */ - for (idx = 0; idx < MAX_PACKETS_IN_FLY; idx++) { - if (!packets[idx].constructed()) + for (auto const &p : packets) { + if (!p.constructed()) break; + ++idx; } if (idx == MAX_PACKETS_IN_FLY) return false; @@ -464,10 +481,7 @@ bool genode_usb_session::request(genode_usb_request_callbacks & req, void * data _ack(Packet_descriptor::PACKET_INVALID_ERROR, p); } - addr_t offset = (addr_t)sink()->packet_content(p) - - (addr_t)sink()->ds_local_base(); - void * addr = (void*)(genode_shared_dataspace_local_address(_ds) - + offset); + genode_usb_buffer payload = _buffer_of_packet(p); packets[idx].construct(p); _id.inc(); /* increment the session ids usage */ @@ -475,19 +489,19 @@ bool genode_usb_session::request(genode_usb_request_callbacks & req, void * data switch (p.type) { case Packet_descriptor::STRING: req.string_fn((genode_usb_request_string*)&p.string, - _id.id(), idx, addr, p.size(), data); + _id.id(), idx, payload, data); break; case Packet_descriptor::CTRL: - req.urb_fn({ CTRL, &p.control }, _id.id(), idx, addr, p.size(), data); + req.urb_fn({ CTRL, &p.control }, _id.id(), idx, payload, data); break; case Packet_descriptor::BULK: - req.urb_fn({ BULK, &p.transfer }, _id.id(), idx, addr, p.size(), data); + req.urb_fn({ BULK, &p.transfer }, _id.id(), idx, payload, data); break; case Packet_descriptor::IRQ: - req.urb_fn({ IRQ, &p.transfer }, _id.id(), idx, addr, p.size(), data); + req.urb_fn({ IRQ, &p.transfer }, _id.id(), idx, payload, data); break; case Packet_descriptor::ISOC: - req.urb_fn({ ISOC, &p.transfer }, _id.id(), idx, addr, p.size(), data); + req.urb_fn({ ISOC, &p.transfer }, _id.id(), idx, payload, data); break; case Packet_descriptor::ALT_SETTING: req.altsetting_fn(p.interface.number, p.interface.alt_setting, @@ -516,22 +530,23 @@ void genode_usb_session::handle_response(genode_usb_request_handle_t id, { using Packet_descriptor = Usb::Packet_descriptor; - Packet_descriptor p = *packets[id]; + Packet_descriptor p = *packets[id]; + genode_usb_buffer payload = _buffer_of_packet(p); switch (p.type) { case Packet_descriptor::CTRL: - _ack(callback({ CTRL, &p.control }, callback_data), p); + _ack(callback({ CTRL, &p.control }, payload, callback_data), p); break; case Packet_descriptor::BULK: - _ack(callback({ BULK, &p.transfer }, callback_data), p); + _ack(callback({ BULK, &p.transfer }, payload, callback_data), p); break; case Packet_descriptor::IRQ: - _ack(callback({ IRQ, &p.transfer }, callback_data), p); + _ack(callback({ IRQ, &p.transfer }, payload, callback_data), p); break; case Packet_descriptor::ISOC: - _ack(callback({ ISOC, &p.transfer }, callback_data), p); + _ack(callback({ ISOC, &p.transfer }, payload, callback_data), p); break; default: - _ack(callback({ NONE, nullptr }, callback_data), p); + _ack(callback({ NONE, nullptr }, { nullptr, 0 }, callback_data), p); }; packets[id].destruct(); } @@ -750,18 +765,18 @@ void ::Root::announce_device(genode_usb_vendor_id_t vendor, genode_usb_bus_num_t bus, genode_usb_dev_num_t dev) { - for (unsigned idx = 0; idx < MAX_DEVICES; idx++) { - if (_devices[idx].constructed()) + for (auto &device : _devices) { + if (device.constructed()) continue; - _devices[idx].construct(vendor, product, cla, bus, dev); + device.construct(vendor, product, cla, bus, dev); _announce_service(); _report(); _for_each_session([&] (genode_usb_session & s) { - if (_devices[idx]->usb_session || !_matches(*_devices[idx], s)) + if (device->usb_session || !_matches(*device, s)) return; - _devices[idx]->usb_session = &s; + device->usb_session = &s; s.notify(); }); return; @@ -774,18 +789,18 @@ void ::Root::announce_device(genode_usb_vendor_id_t vendor, void ::Root::discontinue_device(genode_usb_bus_num_t bus, genode_usb_dev_num_t dev) { - for (unsigned idx = 0; idx < MAX_DEVICES; idx++) { - if (!_devices[idx].constructed() || - _devices[idx]->bus != bus || - _devices[idx]->dev != dev) + for (auto &device : _devices) { + if (!device.constructed() || + device->bus != bus || + device->dev != dev) continue; - if (_devices[idx]->usb_session) { - _devices[idx]->usb_session->notify(); - _devices[idx]->usb_session->flush_packet_stream(); + if (device->usb_session) { + device->usb_session->notify(); + device->usb_session->flush_packet_stream(); } - _devices[idx].destruct(); + device.destruct(); _report(); return; }