On 05/03/20 09:31, Philippe Mathieu-Daudé wrote:
> This series fixes a dangerous zero-length array use.
> Simples patches first to clean the issue in the last patch:
> dissociate the buffer holding DMA requests with pointer to
> SRP Information Unit packets.
>
> v2: Addressed David Gibson review comments
>
> Philippe Mathieu-Daudé (5):
> hw/scsi/viosrp: Add missing 'hw/scsi/srp.h' include
> hw/scsi/spapr_vscsi: Use SRP_MAX_IU_LEN instead of sizeof flexible
> array
> hw/scsi/spapr_vscsi: Simplify a bit
> hw/scsi/spapr_vscsi: Introduce req_iu() helper
> hw/scsi/spapr_vscsi: Do not mix SRP IU size with DMA buffer size
>
> hw/scsi/viosrp.h | 3 ++-
> hw/scsi/spapr_vscsi.c | 59 ++++++++++++++++++++++++-------------------
> 2 files changed, 35 insertions(+), 27 deletions(-)
>
I think this is clearer than the "reserved" member, and it gets one
important thing right though by decoupling SRP_MAX_IU_LEN from
sizeof(union srp_iu).
However it doesn't fix the actual time bomb, which is that,
vscsi_send_rsp can overrun the buffer size here depending on the length
of sense data:
memcpy(req->iu.srp.rsp.data, req->sense, req->senselen);
But with this series in place we can fix that with just a couple of
asserts, like this:
/* This #define is why Phil's changes matter! */
#define SRP_MAX_IU_DATA_LEN (SRP_MAX_IU_LEN - sizeof(union srp_iu))
/* in vscsi_send_rsp */
int sense_data_len = MIN(req->senselen, SRP_MAX_IU_DATA_LEN);
req->iu.srp.rsp.sense_data_len = cpu_to_be32(sense_data_len);
memcpy(req->iu.srp.rsp.data, req->sense, sense_data_len);
/* in vscsi_send_iu */
assert(length < SRP_MAX_IU_LEN);
/* in vscsi_process_tsk_mgmt */
QEMU_BUILD_BUG_ON(SRP_MAX_IU_DATA_LEN < 4);
(Easier to write code than describe...).
Paolo
