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

Kees Cook
2025-07-25T21:54:52Z

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