From caef7d642afd7c14e5a986d68fc2dd641b0b2c1c Mon Sep 17 00:00:00 2001 From: Stefan Kalkowski Date: Thu, 23 Feb 2023 16:38:29 +0100 Subject: [PATCH] usb_block: enable WARN_STRICT_CONVERSION switch Implicitely fixes problems with USB devices having more than 4G blocks. Formerly the 16-Cmd LBA requests were silently casted to 32-bit. Fix genodelabs/genode#4771 --- repos/os/src/drivers/usb_block/cbw_csw.h | 9 ++-- repos/os/src/drivers/usb_block/main.cc | 56 +++++++++++++------- repos/os/src/drivers/usb_block/scsi.h | 67 ++++++++++-------------- repos/os/src/drivers/usb_block/target.mk | 2 - 4 files changed, 70 insertions(+), 64 deletions(-) diff --git a/repos/os/src/drivers/usb_block/cbw_csw.h b/repos/os/src/drivers/usb_block/cbw_csw.h index 6c87f2a5e1..59715cd2db 100644 --- a/repos/os/src/drivers/usb_block/cbw_csw.h +++ b/repos/os/src/drivers/usb_block/cbw_csw.h @@ -23,6 +23,7 @@ namespace Usb { using Genode::uint8_t; +using Genode::uint16_t; using Genode::uint32_t; using Genode::uint64_t; using Genode::size_t; @@ -161,7 +162,7 @@ struct Read_capacity_10 : Usb::Cbw, Scsi::Read_capacity_10 struct Read_10 : Usb::Cbw, Scsi::Read_10 { Read_10(addr_t addr, uint32_t tag, uint8_t lun, - uint32_t lba, uint32_t len, uint32_t block_size) + uint32_t lba, uint16_t len, uint32_t block_size) : Cbw(addr, tag, len * block_size, Usb::ENDPOINT_IN, lun, Scsi::Read_10::LENGTH), @@ -180,7 +181,7 @@ struct Read_10 : Usb::Cbw, Scsi::Read_10 struct Write_10 : Usb::Cbw, Scsi::Write_10 { Write_10(addr_t addr, uint32_t tag, uint8_t lun, - uint32_t lba, uint32_t len, uint32_t block_size) + uint32_t lba, uint16_t len, uint32_t block_size) : Cbw(addr, tag, len * block_size, Usb::ENDPOINT_OUT, lun, Scsi::Write_10::LENGTH), @@ -217,7 +218,7 @@ struct Read_capacity_16 : Usb::Cbw, Scsi::Read_capacity_16 struct Read_16 : Usb::Cbw, Scsi::Read_16 { Read_16(addr_t addr, uint32_t tag, uint8_t lun, - uint32_t lba, uint32_t len, uint32_t block_size) + uint64_t lba, uint32_t len, uint32_t block_size) : Cbw(addr, tag, len * block_size, Usb::ENDPOINT_IN, lun, Scsi::Read_16::LENGTH), @@ -236,7 +237,7 @@ struct Read_16 : Usb::Cbw, Scsi::Read_16 struct Write_16 : Usb::Cbw, Scsi::Write_16 { Write_16(addr_t addr, uint32_t tag, uint8_t lun, - uint32_t lba, uint32_t len, uint32_t block_size) + uint64_t lba, uint32_t len, uint32_t block_size) : Cbw(addr, tag, len * block_size, Usb::ENDPOINT_OUT, lun, Scsi::Write_16::LENGTH), diff --git a/repos/os/src/drivers/usb_block/main.cc b/repos/os/src/drivers/usb_block/main.cc index c1693d8992..bf7f4c3423 100644 --- a/repos/os/src/drivers/usb_block/main.cc +++ b/repos/os/src/drivers/usb_block/main.cc @@ -142,12 +142,15 @@ struct Usb::Block_driver : Usb::Completion return usb_label; } + enum Sizes : size_t { PACKET_STREAM_BUF_SIZE = 2 * (1UL << 20) }; + /* * USB session */ Allocator_avl alloc; Usb::Connection usb { env, &alloc, - get_label<128>(config.xml()).string(), 2 * (1<<20), state_change_dispatcher }; + get_label<128>(config.xml()).string(), PACKET_STREAM_BUF_SIZE, + state_change_dispatcher }; Usb::Device device; Signal_handler
&block_request_handler; @@ -161,7 +164,7 @@ struct Usb::Block_driver : Usb::Completion * Block session */ Block::sector_t _block_count { 0 }; - size_t _block_size { 0 }; + uint32_t _block_size { 0 }; bool _writeable = false; @@ -204,7 +207,7 @@ struct Usb::Block_driver : Usb::Completion uint8_t interface; Block::sector_t block_count = 0; - size_t block_size = 0; + uint32_t block_size = 0; char vendor[Scsi::Inquiry_response::Vid::ITEMS+1]; char product[Scsi::Inquiry_response::Pid::ITEMS+1]; @@ -328,8 +331,8 @@ struct Usb::Block_driver : Usb::Completion break; } - uint32_t const tag = csw.tag(); - uint8_t const status = csw.sts(); + uint32_t const tag = csw.tag(); + uint32_t const status = csw.sts(); if (status != Csw::PASSED) { error("CSW failed: ", Hex(status, Hex::PREFIX, Hex::PAD), " tag: ", tag); @@ -451,7 +454,7 @@ struct Usb::Block_driver : Usb::Completion /* cap value in case there is no bulk-only */ active_alt_setting = 0; - for (unsigned i = 0; i < iface.alternate_count(); i++) { + for (uint16_t i = 0; i < iface.alternate_count(); i++) { Alternate_interface &aif = iface.alternate_interface(i); if (aif.iclass == ICLASS_MASS_STORAGE && aif.isubclass == ISUBCLASS_SCSI @@ -480,7 +483,7 @@ struct Usb::Block_driver : Usb::Completion return false; } - for (int i = 0; i < alt_iface.num_endpoints; i++) { + for (uint8_t i = 0; i < alt_iface.num_endpoints; i++) { Endpoint ep = alt_iface.endpoint(i); if (!ep.bulk()) continue; @@ -617,9 +620,12 @@ struct Usb::Block_driver : Usb::Completion throw -1; } - if (init.block_count == 0x100000000) { + /** + * The READ_CAPACITY_10 count is 32-bit last() block + 1. + * If the maximum value is reached READ_CAPACITY_16 has to be used. + */ + if (init.block_count > ~(uint32_t)0U) { - /* capacity too large, try Scsi::Opcode::READ_CAPACITY_16 next */ Read_capacity_16 read_cap((addr_t)cbw_buffer, CAP_TAG, active_lun); init.read_capacity = false; @@ -789,7 +795,7 @@ struct Usb::Block_driver : Usb::Completion break; } - uint8_t const status = csw.sts(); + uint32_t const status = csw.sts(); if (status != Csw::PASSED) { error("CSW failed: ", Hex(status, Hex::PREFIX, Hex::PAD), " read: ", request->read(), " buffer: ", (void *)request->address, @@ -818,14 +824,13 @@ struct Usb::Block_driver : Usb::Completion { _writeable = node.attribute_value("writeable", false); _report_device = node.attribute_value("report", false); - active_interface = node.attribute_value("interface", 0UL); - active_lun = node.attribute_value("lun", 0UL); + active_interface = node.attribute_value("interface", 0); + active_lun = node.attribute_value("lun", 0); reset_device = node.attribute_value("reset_device", false); verbose_scsi = node.attribute_value("verbose_scsi", false); active_alt_setting = - node.attribute_value("alt_setting", - (unsigned long)INVALID_ALT_SETTING); + node.attribute_value("alt_setting", INVALID_ALT_SETTING); } /** @@ -862,13 +867,24 @@ struct Usb::Block_driver : Usb::Completion { uint32_t const t = new_tag(); + /** + * Assuming a minimal packet size of 512. Total packet stream buffer + * should not exceed 16-bit block count value. + */ + static_assert((PACKET_STREAM_BUF_SIZE / 512UL) < (uint16_t)~0UL); + uint16_t c = (uint16_t) count; + + /** + * We check for lba fitting 32-bit value for 10-Cmd mode + * before entering this function + */ char cb[Cbw::LENGTH]; if (read) { - if (force_cmd_16) Read_16 r((addr_t)cb, t, active_lun, lba, count, _block_size); - else Read_10 r((addr_t)cb, t, active_lun, lba, count, _block_size); + if (force_cmd_16) Read_16 r((addr_t)cb, t, active_lun, lba, c, _block_size); + else Read_10 r((addr_t)cb, t, active_lun, (uint32_t)lba, c, _block_size); } else { - if (force_cmd_16) Write_16 w((addr_t)cb, t, active_lun, lba, count, _block_size); - else Write_10 w((addr_t)cb, t, active_lun, lba, count, _block_size); + if (force_cmd_16) Write_16 w((addr_t)cb, t, active_lun, lba, c, _block_size); + else Write_10 w((addr_t)cb, t, active_lun, (uint32_t)lba, c, _block_size); } cbw(cb, *this); @@ -904,6 +920,10 @@ struct Usb::Block_driver : Usb::Completion if (last > info().block_count) return Response::REJECTED; + /* we only support 32-bit block numbers in 10-Cmd mode */ + if (!force_cmd_16 && last >= ~0U) + return Response::REJECTED; + /* check if request is pending */ if (request_pending()) return Response::RETRY; diff --git a/repos/os/src/drivers/usb_block/scsi.h b/repos/os/src/drivers/usb_block/scsi.h index c4510fc0d3..9d7e23b8e4 100644 --- a/repos/os/src/drivers/usb_block/scsi.h +++ b/repos/os/src/drivers/usb_block/scsi.h @@ -22,9 +22,20 @@ namespace Scsi { using namespace Genode; - uint16_t be16(uint16_t val); - uint32_t be32(uint32_t val); - uint64_t be64(uint64_t val); + /******************* + ** Endian helper ** + *******************/ + + template + T be(T val) + { + uint8_t * p = reinterpret_cast(&val); + T ret = 0; + for (size_t i = 0; i < sizeof(T); i++) + ret |= (T) (p[i] << ((sizeof(T)-i-1)*8)); + return ret; + } + /****************** * SCSI commands ** @@ -69,30 +80,6 @@ namespace Scsi { } -/******************* -** Endian helper ** -*******************/ - -Genode::uint16_t Scsi::be16(Genode::uint16_t val) -{ - Genode::uint8_t *p = reinterpret_cast(&val); - return (p[1]<<0)|(p[0]<<8); -} - -Genode::uint32_t Scsi::be32(Genode::uint32_t val) -{ - Genode::uint8_t *p = reinterpret_cast(&val); - return (p[3]<<0)|(p[2]<<8)|(p[1]<<16)|(p[0]<<24); -} - -Genode::uint64_t Scsi::be64(Genode::uint64_t val) -{ - Genode::uint8_t *p = reinterpret_cast(&val); - return ((((Genode::uint64_t)(p[3]<<0)|(p[2]<<8)|(p[1]<<16)|(p[0]<<24))<<32)| - (((Genode::uint32_t)(p[7]<<0)|(p[6]<<8)|(p[5]<<16)|(p[4]<<24)))); -} - - /*************************** * SCSI command responses ** ***************************/ @@ -189,8 +176,8 @@ struct Scsi::Capacity_response_10 : Genode::Mmio Capacity_response_10(addr_t addr) : Mmio(addr) { } - uint32_t last_block() const { return be32(read()); } - uint32_t block_size() const { return be32(read()); } + uint32_t last_block() const { return be(read()); } + uint32_t block_size() const { return be(read()); } void dump() { @@ -210,8 +197,8 @@ struct Scsi::Capacity_response_16 : Genode::Mmio Capacity_response_16(addr_t addr) : Mmio(addr) { } - uint64_t last_block() const { return be64(read()); } - uint32_t block_size() const { return be32(read()); } + uint64_t last_block() const { return be(read()); } + uint32_t block_size() const { return be(read()); } void dump() { @@ -239,7 +226,7 @@ struct Scsi::Cmd_6 : Genode::Mmio void dump() { Genode::log("Op: ", Genode::Hex(read())); - Genode::log("Lba: ", Genode::Hex(be16(read()))); + Genode::log("Lba: ", Genode::Hex(be(read()))); Genode::log("Len: ", read()); Genode::log("Ctl: ", Genode::Hex(read())); } @@ -327,8 +314,8 @@ struct Scsi::Cmd_10 : Genode::Mmio void dump() { Genode::log("Op: ", Genode::Hex(read())); - Genode::log("Lba: ", Genode::Hex(be32(read()))); - Genode::log("Len: ", be16(read())); + Genode::log("Lba: ", Genode::Hex(be(read()))); + Genode::log("Len: ", be(read())); Genode::log("Ctl: ", Genode::Hex(read())); } }; @@ -347,8 +334,8 @@ struct Scsi::Io_10 : Cmd_10 { Io_10(addr_t addr, uint32_t lba, uint16_t len) : Cmd_10(addr) { - write(be32(lba)); - write(be16(len)); + write(be(lba)); + write(be(len)); } }; @@ -388,8 +375,8 @@ struct Scsi::Cmd_16 : Genode::Mmio void dump() { Genode::log("Op: ", Genode::Hex(read())); - Genode::log("Lba: ", Genode::Hex(be64(read()))); - Genode::log("Len: ", be32(read())); + Genode::log("Lba: ", Genode::Hex(be(read()))); + Genode::log("Len: ", be(read())); Genode::log("Ctl: ", Genode::Hex(read())); } }; @@ -408,8 +395,8 @@ struct Scsi::Io_16 : Cmd_16 { Io_16(addr_t addr, uint64_t lba, uint32_t len) : Cmd_16(addr) { - write(be64(lba)); - write(be32(len)); + write(be(lba)); + write(be(len)); } }; diff --git a/repos/os/src/drivers/usb_block/target.mk b/repos/os/src/drivers/usb_block/target.mk index ba1d29725d..a0663d5c01 100644 --- a/repos/os/src/drivers/usb_block/target.mk +++ b/repos/os/src/drivers/usb_block/target.mk @@ -2,5 +2,3 @@ TARGET = usb_block_drv SRC_CC = main.cc INC_DIR = $(PRG_DIR) LIBS = base - -CC_CXX_WARN_STRICT_CONVERSION =