[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 0/5] hw/scsi/spapr_vscsi: Fix time bomb zero-length array
From: |
Paolo Bonzini |
Subject: |
Re: [PATCH v2 0/5] hw/scsi/spapr_vscsi: Fix time bomb zero-length array use |
Date: |
Thu, 5 Mar 2020 11:27:21 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 |
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
- [PATCH v2 0/5] hw/scsi/spapr_vscsi: Fix time bomb zero-length array use, Philippe Mathieu-Daudé, 2020/03/05
- [MERGED PATCH v2 1/5] hw/scsi/viosrp: Add missing 'hw/scsi/srp.h' include, Philippe Mathieu-Daudé, 2020/03/05
- [MERGED PATCH v2 2/5] hw/scsi/spapr_vscsi: Use SRP_MAX_IU_LEN instead of sizeof flexible array, Philippe Mathieu-Daudé, 2020/03/05
- [PATCH v2 4/5] hw/scsi/spapr_vscsi: Introduce req_iu() helper, Philippe Mathieu-Daudé, 2020/03/05
- [MERGED PATCH v2 3/5] hw/scsi/spapr_vscsi: Simplify a bit, Philippe Mathieu-Daudé, 2020/03/05
- [PATCH v2 5/5] hw/scsi/spapr_vscsi: Do not mix SRP IU size with DMA buffer size, Philippe Mathieu-Daudé, 2020/03/05
- Re: [PATCH v2 0/5] hw/scsi/spapr_vscsi: Fix time bomb zero-length array use,
Paolo Bonzini <=