These structures are hardware interfaces, ensure the layout is
correct. Add defines for the data sizes throughout the code.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/usb/dev-storage.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 394fb8e1ec0..41924b9320e 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -27,7 +27,14 @@
 #define MassStorageReset  0xff
 #define GetMaxLun         0xfe
 
-struct usb_msd_cbw {
+/*
+ * CBW and CSW packets have a minimum size, enough to contain the
+ * respective data structure.
+ */
+#define CBW_SIZE sizeof(struct usb_msd_cbw)
+#define CSW_SIZE sizeof(struct usb_msd_csw)
+
+struct QEMU_PACKED usb_msd_cbw {
     uint32_t sig;
     uint32_t tag;
     uint32_t data_len;
@@ -405,11 +412,11 @@ static void usb_msd_handle_data_out(USBDevice *dev, USBPacket *p)
 
     switch (s->mode) {
     case USB_MSDM_CBW:
-        if (p->iov.size != 31) {
+        if (p->iov.size != CBW_SIZE) {
             error_report("usb-msd: Bad CBW size");
             goto fail;
         }
-        usb_packet_copy(p, &cbw, 31);
+        usb_packet_copy(p, &cbw, CBW_SIZE);
         if (le32_to_cpu(cbw.sig) != 0x43425355) {
             error_report("usb-msd: Bad signature %08x",
                          le32_to_cpu(cbw.sig));
@@ -489,7 +496,7 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
 
     switch (s->mode) {
     case USB_MSDM_DATAOUT:
-        if (s->data_len != 0 || p->iov.size < 13) {
+        if (s->data_len != 0 || p->iov.size < CSW_SIZE) {
             goto fail;
         }
         /* Waiting for SCSI write to complete.  */
@@ -499,7 +506,7 @@ static void usb_msd_handle_data_in(USBDevice *dev, USBPacket *p)
         break;
 
     case USB_MSDM_CSW:
-        if (p->iov.size < 13) {
+        if (p->iov.size < CSW_SIZE) {
             goto fail;
         }
 
@@ -636,6 +643,10 @@ static const TypeInfo usb_storage_dev_type_info = {
 
 static void usb_msd_register_types(void)
 {
+    /* Ensure the header structures are the right size */
+    qemu_build_assert(CBW_SIZE == 31);
+    qemu_build_assert(CSW_SIZE == 13);
+
     type_register_static(&usb_storage_dev_type_info);
 }
 
-- 
2.47.1

Am 02.05.2025 um 05:30 hat Nicholas Piggin geschrieben:
> These structures are hardware interfaces, ensure the layout is
> correct. Add defines for the data sizes throughout the code.
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

> @@ -636,6 +643,10 @@ static const TypeInfo usb_storage_dev_type_info = {
>  
>  static void usb_msd_register_types(void)
>  {
> +    /* Ensure the header structures are the right size */
> +    qemu_build_assert(CBW_SIZE == 31);
> +    qemu_build_assert(CSW_SIZE == 13);
> +
>      type_register_static(&usb_storage_dev_type_info);
>  }

There is no real reason to have this assertion inside of a function at
the end of the file. I'd prefer QEMU_BUILD_BUG_ON() next to the struct
declarations, but obviously it's correct either way, so with or without
that changed:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>