From a2bdcc68c22aeb04e1b7d6030f37f298be219aec Mon Sep 17 00:00:00 2001 From: Emery Hemingway Date: Sun, 25 Nov 2018 11:58:58 +0100 Subject: [PATCH] Throw exception for invalid packets at packet streams Some application code is dereferencing the pointer returned by 'packet_content' at packet streams without checking that it is valid. Throw an exception rather than return a null pointer, except for zero-length packets, which have somewhat implicit invalid content and that we believe to be properly handled in all current cases. The client-side of a packet stream cannot take corrective action if the server-side is sending packets with invalid content, but the servers that provide packet streams should catch this exception to detect misbehaving clients. Ref #3059 --- .../src/drivers/nic/fec/component.cc | 2 +- repos/dde_linux/src/drivers/usb_host/raw.cc | 3 +- .../src/drivers/usb_net/component.cc | 2 +- repos/dde_linux/src/lib/lxip/nic_handler.cc | 5 +- .../src/lib/usb/include/usb_nic_component.h | 5 +- repos/dde_linux/src/lib/usb/raw/raw.cc | 2 +- repos/dde_linux/src/lib/wifi/nic.cc | 2 +- repos/dde_rump/src/server/rump_fs/main.cc | 11 ++- repos/os/include/block/component.h | 5 +- repos/os/include/os/packet_stream.h | 68 ++++++++++--------- repos/os/src/drivers/nic/spec/linux/main.cc | 2 +- repos/os/src/drivers/nic/spec/pbxa9/lan9118.h | 2 +- repos/os/src/server/lx_fs/main.cc | 9 ++- .../src/server/nic_bridge/packet_handler.cc | 2 +- repos/os/src/server/nic_dump/interface.cc | 2 +- repos/os/src/server/nic_loopback/main.cc | 4 +- repos/os/src/server/nic_router/interface.cc | 6 ++ repos/os/src/server/part_blk/component.h | 17 +++-- repos/os/src/server/ram_fs/main.cc | 15 ++-- repos/os/src/server/vfs/main.cc | 13 ++-- 20 files changed, 100 insertions(+), 77 deletions(-) diff --git a/repos/dde_linux/src/drivers/nic/fec/component.cc b/repos/dde_linux/src/drivers/nic/fec/component.cc index fdac2710e3..cbe1623c43 100644 --- a/repos/dde_linux/src/drivers/nic/fec/component.cc +++ b/repos/dde_linux/src/drivers/nic/fec/component.cc @@ -82,7 +82,7 @@ bool Session_component::_send() if (!_tx.sink()->packet_avail()) { return false; } Packet_descriptor packet = _tx.sink()->get_packet(); - if (!packet.size()) { + if (!packet.size() || !_tx.sink()->packet_valid(packet)) { warning("invalid tx packet"); return true; } diff --git a/repos/dde_linux/src/drivers/usb_host/raw.cc b/repos/dde_linux/src/drivers/usb_host/raw.cc index f6f48bee2d..8b87397324 100644 --- a/repos/dde_linux/src/drivers/usb_host/raw.cc +++ b/repos/dde_linux/src/drivers/usb_host/raw.cc @@ -497,7 +497,8 @@ class Usb::Worker : public Genode::Weak_object _p_in_flight++; if (!_device || !_device->udev || - _device->udev->state == USB_STATE_NOTATTACHED) { + _device->udev->state == USB_STATE_NOTATTACHED || + !_sink->packet_valid(p)) { _ack_packet(p); continue; } diff --git a/repos/dde_linux/src/drivers/usb_net/component.cc b/repos/dde_linux/src/drivers/usb_net/component.cc index fdac2710e3..cbe1623c43 100644 --- a/repos/dde_linux/src/drivers/usb_net/component.cc +++ b/repos/dde_linux/src/drivers/usb_net/component.cc @@ -82,7 +82,7 @@ bool Session_component::_send() if (!_tx.sink()->packet_avail()) { return false; } Packet_descriptor packet = _tx.sink()->get_packet(); - if (!packet.size()) { + if (!packet.size() || !_tx.sink()->packet_valid(packet)) { warning("invalid tx packet"); return true; } diff --git a/repos/dde_linux/src/lib/lxip/nic_handler.cc b/repos/dde_linux/src/lib/lxip/nic_handler.cc index f2b8c0751b..1039794e8c 100644 --- a/repos/dde_linux/src/lib/lxip/nic_handler.cc +++ b/repos/dde_linux/src/lib/lxip/nic_handler.cc @@ -74,8 +74,9 @@ class Nic_client count++ < MAX_PACKETS) { Nic::Packet_descriptor p = _nic.rx()->get_packet(); - net_driver_rx(_nic.rx()->packet_content(p), p.size()); - + try { net_driver_rx(_nic.rx()->packet_content(p), p.size()); } + catch (Genode::Packet_descriptor::Invalid_packet) { + Genode::error("received invalid Nic packet"); } _nic.rx()->acknowledge_packet(p); } diff --git a/repos/dde_linux/src/lib/usb/include/usb_nic_component.h b/repos/dde_linux/src/lib/usb/include/usb_nic_component.h index c576f4e8e3..97ed484254 100644 --- a/repos/dde_linux/src/lib/usb/include/usb_nic_component.h +++ b/repos/dde_linux/src/lib/usb/include/usb_nic_component.h @@ -131,7 +131,8 @@ class Usb_nic::Session_component : public Nic::Session_component } /* copy packet to current data pos */ - Genode::memcpy(work_skb.data, _tx.sink()->packet_content(packet), packet.size()); + try { Genode::memcpy(work_skb.data, _tx.sink()->packet_content(packet), packet.size()); } + catch (Genode::Packet_descriptor::Invalid_packet) { } /* call fixup on dummy SKB */ _device->tx_fixup(&work_skb); /* advance to next slot */ @@ -155,7 +156,7 @@ class Usb_nic::Session_component : public Nic::Session_component return false; Genode::Packet_descriptor packet = _tx.sink()->get_packet(); - if (!packet.size()) { + if (!packet.size() || !_tx.sink()->packet_valid(packet)) { Genode::warning("Invalid tx packet"); return true; } diff --git a/repos/dde_linux/src/lib/usb/raw/raw.cc b/repos/dde_linux/src/lib/usb/raw/raw.cc index f7a0aaa5f8..239904c8be 100644 --- a/repos/dde_linux/src/lib/usb/raw/raw.cc +++ b/repos/dde_linux/src/lib/usb/raw/raw.cc @@ -482,7 +482,7 @@ class Usb::Worker : public Genode::Weak_object _p_in_flight++; - if (!_device || !_device->udev) { + if (!_device || !_device->udev || !_sink->packet_valid(p)) { _ack_packet(p); continue; } diff --git a/repos/dde_linux/src/lib/wifi/nic.cc b/repos/dde_linux/src/lib/wifi/nic.cc index a5969cb91f..368db2b606 100644 --- a/repos/dde_linux/src/lib/wifi/nic.cc +++ b/repos/dde_linux/src/lib/wifi/nic.cc @@ -138,7 +138,7 @@ class Wifi_session_component : public Nic::Session_component if (!_tx.sink()->packet_avail()) { return false; } Packet_descriptor packet = _tx.sink()->get_packet(); - if (!packet.size()) { + if (!packet.size() || !_tx.sink()->packet_valid(packet)) { warning("invalid tx packet"); return true; } diff --git a/repos/dde_rump/src/server/rump_fs/main.cc b/repos/dde_rump/src/server/rump_fs/main.cc index d0d70698ad..b5abae6617 100644 --- a/repos/dde_rump/src/server/rump_fs/main.cc +++ b/repos/dde_rump/src/server/rump_fs/main.cc @@ -65,7 +65,6 @@ class Rump_fs::Session_component : public Session_rpc_object */ void _process_packet_op(Packet_descriptor &packet, Open_node &open_node) { - void * const content = tx_sink()->packet_content(packet); size_t const length = packet.length(); /* resulting length */ @@ -75,9 +74,9 @@ class Rump_fs::Session_component : public Session_rpc_object switch (packet.operation()) { case Packet_descriptor::READ: - if (content && (packet.length() <= packet.size())) { - res_length = open_node.node().read((char *)content, length, - packet.position()); + if (tx_sink()->packet_valid(packet) && packet.length() <= packet.size()) { + res_length = open_node.node().read((char *)tx_sink()->packet_content(packet), + length, packet.position()); /* read data or EOF is a success */ succeeded = res_length || (packet.position() >= open_node.node().status().size); @@ -85,8 +84,8 @@ class Rump_fs::Session_component : public Session_rpc_object break; case Packet_descriptor::WRITE: - if (content && (packet.length() <= packet.size())) { - res_length = open_node.node().write((char const *)content, + if (tx_sink()->packet_valid(packet) && packet.length() <= packet.size()) { + res_length = open_node.node().write((char const *)tx_sink()->packet_content(packet), length, packet.position()); /* File system session can't handle partial writes */ diff --git a/repos/os/include/block/component.h b/repos/os/include/block/component.h index 54c2c270c8..86429b9b91 100644 --- a/repos/os/include/block/component.h +++ b/repos/os/include/block/component.h @@ -106,7 +106,7 @@ class Block::Session_component : public Block::Session_component_base, _p_to_handle.succeeded(false); /* ignore invalid packets */ - if (!packet.size() || !_range_check(_p_to_handle)) { + if (!packet.size() || !_range_check(_p_to_handle) || !tx_sink()->packet_valid(packet)) { _ack_packet(_p_to_handle); return; } @@ -151,6 +151,9 @@ class Block::Session_component : public Block::Session_component_base, _req_queue_full = true; } catch (Driver::Io_error) { _ack_packet(_p_to_handle); + } catch (Genode::Packet_descriptor::Invalid_packet) { + _p_to_handle = Packet_descriptor(); + Genode::error("dropping invalid Block packet"); } } diff --git a/repos/os/include/os/packet_stream.h b/repos/os/include/os/packet_stream.h index 256119044a..3a797d32ea 100644 --- a/repos/os/include/os/packet_stream.h +++ b/repos/os/include/os/packet_stream.h @@ -129,6 +129,11 @@ class Genode::Packet_descriptor */ enum Alignment { PACKET_ALIGNMENT = 0 }; + /** + * Exception type thrown by packet streams + */ + class Invalid_packet { }; + /** * Constructor */ @@ -530,6 +535,24 @@ class Genode::Packet_stream_base * Return communication buffer */ Genode::Dataspace_capability _dataspace() { return _ds_cap; } + + bool packet_valid(Packet_descriptor packet) + { + return (packet.offset() >= _bulk_buffer_offset + && packet.offset() < _bulk_buffer_offset + (Genode::off_t)_bulk_buffer_size + && packet.offset() + packet.size() <= _bulk_buffer_offset + _bulk_buffer_size); + } + + template + CONTENT_TYPE *packet_content(Packet_descriptor packet) + { + if (!packet.size()) return nullptr; + + if (!packet_valid(packet) || packet.size() < sizeof(CONTENT_TYPE)) + throw Packet_descriptor::Invalid_packet(); + + return (CONTENT_TYPE *)((Genode::addr_t)_ds_local_base + packet.offset()); + } }; @@ -621,6 +644,8 @@ class Genode::Packet_stream_source : private Packet_stream_base _bulk_buffer_size); } + using Packet_stream_base::packet_valid; + /** * Return the size of the bulk buffer. */ @@ -680,25 +705,14 @@ class Genode::Packet_stream_source : private Packet_stream_base return Packet_descriptor((Genode::off_t)base, size); } - bool packet_valid(Packet_descriptor packet) - { - return (packet.offset() >= _bulk_buffer_offset - && packet.offset() < _bulk_buffer_offset + (Genode::off_t)_bulk_buffer_size - && packet.offset() + packet.size() <= _bulk_buffer_offset + _bulk_buffer_size); - } - /** * Get pointer to the content of the specified packet * - * \return 0 if the packet is invalid + * \throw Packet_descriptor::Invalid_packet raise an exception if + the packet is invalid */ - Content_type *packet_content(Packet_descriptor packet) - { - if (!packet_valid(packet) || packet.size() < sizeof(Content_type)) - return 0; - - return (Content_type *)((Genode::addr_t)_ds_local_base + packet.offset()); - } + Content_type *packet_content(Packet_descriptor packet) { + return Packet_stream_base::packet_content(packet); } /** * Return true if submit queue can hold another packet @@ -786,6 +800,8 @@ class Genode::Packet_stream_sink : private Packet_stream_base Ack_queue::PRODUCER)) { } + using Packet_stream_base::packet_valid; + /** * Register signal handler to notify that new acknowledgements * are available in the ack queue. @@ -827,16 +843,6 @@ class Genode::Packet_stream_sink : private Packet_stream_base */ bool packet_avail() { return _submit_receiver.ready_for_rx(); } - /** - * Check if packet descriptor refers to a range within the bulk buffer - */ - bool packet_valid(Packet_descriptor packet) - { - return (packet.offset() >= _bulk_buffer_offset - && packet.offset() < _bulk_buffer_offset + (Genode::off_t)_bulk_buffer_size - && packet.offset() + packet.size() <= _bulk_buffer_offset + _bulk_buffer_size); - } - /** * Get next packet from source * @@ -862,15 +868,11 @@ class Genode::Packet_stream_sink : private Packet_stream_base /** * Get pointer to the content of the specified packet * - * \return 0 if the packet is invalid + * \throw Packet_descriptor::Invalid_packet raise an exception if + the packet is invalid */ - Content_type *packet_content(Packet_descriptor packet) - { - if (!packet_valid(packet) || packet.size() < sizeof(Content_type)) - return 0; - - return (Content_type *)((Genode::addr_t)_ds_local_base + packet.offset()); - } + Content_type *packet_content(Packet_descriptor packet) { + return Packet_stream_base::packet_content(packet); } /** * Returns true if no further acknowledgements can be submitted diff --git a/repos/os/src/drivers/nic/spec/linux/main.cc b/repos/os/src/drivers/nic/spec/linux/main.cc index 810a3cbc6b..2a13c31b6b 100644 --- a/repos/os/src/drivers/nic/spec/linux/main.cc +++ b/repos/os/src/drivers/nic/spec/linux/main.cc @@ -143,7 +143,7 @@ class Linux_session_component : public Nic::Session_component return false; Packet_descriptor packet = _tx.sink()->get_packet(); - if (!packet.size()) { + if (!packet.size() || !_tx.sink()->packet_valid(packet)) { warning("invalid tx packet"); return true; } diff --git a/repos/os/src/drivers/nic/spec/pbxa9/lan9118.h b/repos/os/src/drivers/nic/spec/pbxa9/lan9118.h index 9c63cb76d1..7ad3084fd4 100644 --- a/repos/os/src/drivers/nic/spec/pbxa9/lan9118.h +++ b/repos/os/src/drivers/nic/spec/pbxa9/lan9118.h @@ -204,7 +204,7 @@ class Lan9118 : public Nic::Session_component return false; Genode::Packet_descriptor packet = _tx.sink()->get_packet(); - if (!packet.size()) { + if (!packet.size() || !_tx.sink()->packet_valid(packet)) { Genode::warning("Invalid tx packet"); return true; } diff --git a/repos/os/src/server/lx_fs/main.cc b/repos/os/src/server/lx_fs/main.cc index 3ace25621e..9a35de6101 100644 --- a/repos/os/src/server/lx_fs/main.cc +++ b/repos/os/src/server/lx_fs/main.cc @@ -62,7 +62,6 @@ class Lx_fs::Session_component : public Session_rpc_object */ void _process_packet_op(Packet_descriptor &packet, Open_node &open_node) { - void * const content = tx_sink()->packet_content(packet); size_t const length = packet.length(); /* resulting length */ @@ -72,8 +71,8 @@ class Lx_fs::Session_component : public Session_rpc_object switch (packet.operation()) { case Packet_descriptor::READ: - if (content && (packet.length() <= packet.size())) { - res_length = open_node.node().read((char *)content, length, + if (tx_sink()->packet_valid(packet) && (packet.length() <= packet.size())) { + res_length = open_node.node().read((char *)tx_sink()->packet_content(packet), length, packet.position()); /* read data or EOF is a success */ @@ -82,8 +81,8 @@ class Lx_fs::Session_component : public Session_rpc_object break; case Packet_descriptor::WRITE: - if (content && (packet.length() <= packet.size())) { - res_length = open_node.node().write((char const *)content, + if (tx_sink()->packet_valid(packet) && (packet.length() <= packet.size())) { + res_length = open_node.node().write((char const *)tx_sink()->packet_content(packet), length, packet.position()); diff --git a/repos/os/src/server/nic_bridge/packet_handler.cc b/repos/os/src/server/nic_bridge/packet_handler.cc index 9db4eea216..67d5cd8a09 100644 --- a/repos/os/src/server/nic_bridge/packet_handler.cc +++ b/repos/os/src/server/nic_bridge/packet_handler.cc @@ -28,7 +28,7 @@ void Packet_handler::_ready_to_submit() /* as long as packets are available, and we can ack them */ while (sink()->packet_avail()) { _packet = sink()->get_packet(); - if (!_packet.size()) continue; + if (!_packet.size() || !sink()->packet_valid(_packet)) continue; handle_ethernet(sink()->packet_content(_packet), _packet.size()); if (!sink()->ready_to_ack()) { diff --git a/repos/os/src/server/nic_dump/interface.cc b/repos/os/src/server/nic_dump/interface.cc index 8e3f522e5e..c825cb009c 100644 --- a/repos/os/src/server/nic_dump/interface.cc +++ b/repos/os/src/server/nic_dump/interface.cc @@ -70,7 +70,7 @@ void Net::Interface::_ready_to_submit() while (_sink().packet_avail()) { Packet_descriptor const pkt = _sink().get_packet(); - if (!pkt.size()) { + if (!pkt.size() || !_sink().packet_valid(pkt)) { continue; } _handle_eth(_sink().packet_content(pkt), pkt.size(), pkt); diff --git a/repos/os/src/server/nic_loopback/main.cc b/repos/os/src/server/nic_loopback/main.cc index d585b0e48c..e75e22d394 100644 --- a/repos/os/src/server/nic_loopback/main.cc +++ b/repos/os/src/server/nic_loopback/main.cc @@ -120,8 +120,8 @@ void Nic_loopback::Session_component::_handle_packet_stream() /* obtain packet */ Packet_descriptor const packet_from_client = _tx.sink()->get_packet(); - if (!packet_from_client.size()) { - warning("received zero-size packet"); + if (!packet_from_client.size() || !_tx.sink()->packet_valid(packet_from_client)) { + warning("received invalid packet"); _rx.source()->release_packet(packet_to_client); continue; } diff --git a/repos/os/src/server/nic_router/interface.cc b/repos/os/src/server/nic_router/interface.cc index df4a8c7bd1..48237490ef 100644 --- a/repos/os/src/server/nic_router/interface.cc +++ b/repos/os/src/server/nic_router/interface.cc @@ -1379,6 +1379,7 @@ void Interface::_handle_pkt() _ack_packet(pkt); } catch (Packet_postponed) { } + catch (Genode::Packet_descriptor::Invalid_packet) { } } @@ -1410,6 +1411,11 @@ void Interface::_continue_handle_eth(Domain const &domain, if (domain.verbose_packet_drop()) { log("[", domain, "] drop packet (handling postponed twice)"); } } + catch (Genode::Packet_descriptor::Invalid_packet) { + if (domain.verbose_packet_drop()) { + log("[", domain, "] invalid Nic packet received"); + } + } _ack_packet(pkt); } diff --git a/repos/os/src/server/part_blk/component.h b/repos/os/src/server/part_blk/component.h index cc4fba4f73..d5ab5cd450 100644 --- a/repos/os/src/server/part_blk/component.h +++ b/repos/os/src/server/part_blk/component.h @@ -92,7 +92,6 @@ class Block::Session_component : public Block::Session_rpc_object, bool write = _p_to_handle.operation() == Packet_descriptor::WRITE; sector_t off = _p_to_handle.block_number() + _partition->lba; size_t cnt = _p_to_handle.block_count(); - void* addr = tx_sink()->packet_content(_p_to_handle); if (write && !_writeable) { _ack_packet(_p_to_handle); @@ -100,12 +99,17 @@ class Block::Session_component : public Block::Session_rpc_object, } try { - _driver.io(write, off, cnt, addr, *this, _p_to_handle); + _driver.io(write, off, cnt, + tx_sink()->packet_content(_p_to_handle), + *this, _p_to_handle); } catch (Block::Session::Tx::Source::Packet_alloc_failed) { if (!_req_queue_full) { _req_queue_full = true; Session_component::wait_queue().insert(this); } + } catch (Genode::Packet_descriptor::Invalid_packet) { + Genode::error("dropping invalid Block packet"); + _p_to_handle = Packet_descriptor(); } } @@ -172,14 +176,19 @@ class Block::Session_component : public Block::Session_rpc_object, void dispatch(Packet_descriptor &request, Packet_descriptor &reply) { + request.succeeded(reply.succeeded()); + if (request.operation() == Block::Packet_descriptor::READ) { void *src = _driver.session().tx()->packet_content(reply); Genode::size_t sz = request.block_count() * _driver.blk_size(); - Genode::memcpy(tx_sink()->packet_content(request), src, sz); + try { Genode::memcpy(tx_sink()->packet_content(request), src, sz); } + catch (Genode::Packet_descriptor::Invalid_packet) { + request.succeeded(false); + } } - request.succeeded(reply.succeeded()); + _ack_packet(request); if (_ack_queue_full) diff --git a/repos/os/src/server/ram_fs/main.cc b/repos/os/src/server/ram_fs/main.cc index d2c8f4227c..32807eec78 100644 --- a/repos/os/src/server/ram_fs/main.cc +++ b/repos/os/src/server/ram_fs/main.cc @@ -67,7 +67,6 @@ class Ram_fs::Session_component : public File_system::Session_rpc_object */ void _process_packet_op(Packet_descriptor &packet, Open_node &open_node) { - void * const content = tx_sink()->packet_content(packet); size_t const length = packet.length(); /* resulting length */ @@ -77,12 +76,12 @@ class Ram_fs::Session_component : public File_system::Session_rpc_object switch (packet.operation()) { case Packet_descriptor::READ: - if (content && (packet.length() <= packet.size())) { + if (packet.length() <= packet.size()) { Locked_ptr node { open_node.node() }; if (!node.valid()) break; - res_length = node->read((char *)content, length, - packet.position()); + res_length = node->read((char *)tx_sink()->packet_content(packet), + length, packet.position()); /* read data or EOF is a success */ succeeded = res_length || (packet.position() >= node->status().size); @@ -90,12 +89,12 @@ class Ram_fs::Session_component : public File_system::Session_rpc_object break; case Packet_descriptor::WRITE: - if (content && (packet.length() <= packet.size())) { + if (packet.length() <= packet.size()) { Locked_ptr node { open_node.node() }; if (!node.valid()) break; - res_length = node->write((char const *)content, length, - packet.position()); + res_length = node->write((char const *)tx_sink()->packet_content(packet), + length, packet.position()); /* File system session can't handle partial writes */ if (res_length != length) { @@ -150,6 +149,8 @@ class Ram_fs::Session_component : public File_system::Session_rpc_object } catch (Id_space::Unknown_id const &) { Genode::error("Invalid_handle"); tx_sink()->acknowledge_packet(packet); + } catch (Genode::Packet_descriptor::Invalid_packet) { + Genode::error("dropping invalid File_system packet"); } } diff --git a/repos/os/src/server/vfs/main.cc b/repos/os/src/server/vfs/main.cc index 5f6bc43d68..3e589105be 100644 --- a/repos/os/src/server/vfs/main.cc +++ b/repos/os/src/server/vfs/main.cc @@ -140,13 +140,12 @@ class Vfs_server::Session_component : public File_system::Session_rpc_object, */ void _process_packet_op(Packet_descriptor &packet) { - void * const content = tx_sink()->packet_content(packet); - size_t const length = packet.length(); - seek_off_t const seek = packet.position(); - /* assume failure by default */ packet.succeeded(false); + size_t const length = packet.length(); + seek_off_t const seek = packet.position(); + if ((packet.length() > packet.size())) return; @@ -166,7 +165,8 @@ class Vfs_server::Session_component : public File_system::Session_rpc_object, } if (node.mode() & READ_ONLY) { - res_length = node.read((char *)content, length, seek); + res_length = node.read( + (char *)tx_sink()->packet_content(packet), length, seek); /* no way to distinguish EOF from unsuccessful reads, both have res_length == 0 */ succeeded = true; @@ -184,7 +184,8 @@ class Vfs_server::Session_component : public File_system::Session_rpc_object, try { _apply(packet.handle(), [&] (Io_node &node) { if (node.mode() & WRITE_ONLY) { - res_length = node.write((char const *)content, length, seek); + res_length = node.write( + (char const *)tx_sink()->packet_content(packet), length, seek); /* File system session can't handle partial writes */ if (res_length != length) {