Re: [PATCH 1/2] scsi: qla2xxx: replace non-standard flexible array purex_item.iocb

[Kees Cook <kees@kernel.org>]

On Fri, Jul 25, 2025 at 02:27:31PM -0700, Chris Leech wrote:
> This is defined as a 64-element u8 array, but 64 is the minimum size and
> it can be allocated larger. I don't know why the array was wrapped as a
> single element struct of the same name.
> 
> Replace with a union around DECLARE_FLEX_ARRAY and padding to maintain
> sizeof(struct purex_item) and associated use.
> 
> This was triggering a field-spanning write warning during FPIN testing
> https://lore.kernel.org/linux-nvme/20250709211919.49100-1-bgurney@redhat.com/
> 
>   >  kernel: memcpy: detected field-spanning write (size 60) of single field
>   >  "((uint8_t *)fpin_pkt + buffer_copy_offset)"
>   >  at drivers/scsi/qla2xxx/qla_isr.c:1221 (size 44)

I think this is:

                                memcpy(((uint8_t *)fpin_pkt +
                                    buffer_copy_offset), new_pkt->data,
                                    no_bytes);

I was briefly confused since fpin_pkt is "void *", but I see the
bounds information comes from this assignment: 

        item = qla24xx_alloc_purex_item(vha, total_bytes);
        if (!item)
                return item;

        fpin_pkt = &item->iocb;

> Tested-by: Bryan Gurney <bgurney@redhat.com>
> Signed-off-by: Chris Leech <cleech@redhat.com>
> ---
>  drivers/scsi/qla2xxx/qla_def.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
> index cb95b7b12051d..6237fefeca149 100644
> --- a/drivers/scsi/qla2xxx/qla_def.h
> +++ b/drivers/scsi/qla2xxx/qla_def.h
> @@ -4890,8 +4890,9 @@ struct purex_item {
>  			     struct purex_item *pkt);
>  	atomic_t in_use;
>  	uint16_t size;
> -	struct {
> -		uint8_t iocb[64];
> +	union {
> +		uint8_t __padding[QLA_DEFAULT_PAYLOAD_SIZE];
> +		DECLARE_FLEX_ARRAY(uint8_t, iocb);
>  	} iocb;
>  };

This won't work, unfortunately, as it seems struct purex_item
is embedded into struct scsi_qla_host. (i.e. try building with
KCFLAGS="-Wflex-array-member-not-at-end" and you should see a warning.)

Maybe qla24xx_alloc_purex_item() could return a union type like:

union purex_item_payload
{
	struct purex_item item;
	struct {
		uint8_t __padding[sizeof(struct purex_item) - 64];
		DECLARE_FLEX_ARRAY(uint8_t, iocb);
	};
};

And refactor the handful of callers? In this particular case:


diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
index fe98c76e9be3..1b3d96d623c1 100644
--- a/drivers/scsi/qla2xxx/qla_isr.c
+++ b/drivers/scsi/qla2xxx/qla_isr.c
@@ -1158,7 +1158,7 @@ qla27xx_copy_fpin_pkt(struct scsi_qla_host *vha, void **pkt,
 	uint16_t no_bytes = 0, total_bytes = 0, pending_bytes = 0;
 	uint16_t buffer_copy_offset = 0;
 	uint16_t entry_count, entry_count_remaining;
-	struct purex_item *item;
+	struct purex_item_blob *blob;
 	void *fpin_pkt = NULL;
 
 	total_bytes = (le16_to_cpu(purex->frame_size) & 0x0FFF)
@@ -1171,11 +1171,11 @@ qla27xx_copy_fpin_pkt(struct scsi_qla_host *vha, void **pkt,
 	       "FPIN ELS, frame_size 0x%x, entry count %d\n",
 	       total_bytes, entry_count);
 
-	item = qla24xx_alloc_purex_item(vha, total_bytes);
-	if (!item)
-		return item;
+	blob = qla24xx_alloc_purex_item(vha, total_bytes);
+	if (!blob)
+		return NULL;
 
-	fpin_pkt = &item->iocb;
+	fpin_pkt = blob->iocb;
 
 	memcpy(fpin_pkt, &purex->els_frame_payload[0], no_bytes);
 	buffer_copy_offset += no_bytes;
@@ -1238,12 +1238,12 @@ qla27xx_copy_fpin_pkt(struct scsi_qla_host *vha, void **pkt,
 			ql_log(ql_log_fatal, vha, 0x508b,
 			       "Dropping partial FPIN, underrun bytes = 0x%x, entry cnts 0x%x\n",
 			       total_bytes, entry_count_remaining);
-			qla24xx_free_purex_item(item);
+			qla24xx_free_purex_item(blob->item);
 			return NULL;
 		}
 	} while (entry_count_remaining > 0);
-	host_to_fcp_swap((uint8_t *)&item->iocb, total_bytes);
-	return item;
+	host_to_fcp_swap((uint8_t *)&blob->iocb, total_bytes);
+	return blob->item;
 }
 
 /**




-- 
Kees Cook