[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3] scsi: esp: check length before dma read
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH v3] scsi: esp: check length before dma read |
Date: |
Wed, 15 Jun 2016 18:34:00 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.0 |
On 15/06/2016 18:16, P J P wrote:
> From: Prasad J Pandit <address@hidden>
>
> While doing DMA read into ESP command buffer 's->cmdbuf', the
> length parameter could exceed the buffer size. Add check to avoid
> OOB access. Also increase the command buffer size to 32, which
> is maximum when 's->do_cmd' is set.
>
> Reported-by: Li Qiang <address@hidden>
> Signed-off-by: Prasad J Pandit <address@hidden>
> ---
> hw/scsi/esp.c | 7 +++++--
> include/hw/scsi/esp.h | 3 ++-
> 2 files changed, 7 insertions(+), 3 deletions(-)
>
> Update as per
> -> https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg04194.html
It's okay, but...
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index 4b94bbc..1208a63 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -249,6 +249,9 @@ static void esp_do_dma(ESPState *s)
> len = s->dma_left;
> if (s->do_cmd) {
> trace_esp_do_dma(s->cmdlen, len);
> + if (len > sizeof(s->cmdbuf) - s->cmdlen) {
> + return;
> + }
... this is not necessary so it can be replaced by an assertion. I can
do that when applying.
Paolo
> s->dma_memory_read(s->dma_opaque, &s->cmdbuf[s->cmdlen], len);
> s->ti_size = 0;
> s->cmdlen = 0;
> @@ -348,7 +351,7 @@ static void handle_ti(ESPState *s)
> s->dma_counter = dmalen;
>
> if (s->do_cmd)
> - minlen = (dmalen < 32) ? dmalen : 32;
> + minlen = (dmalen < ESP_CMDBUF_SZ) ? dmalen : ESP_CMDBUF_SZ;
> else if (s->ti_size < 0)
> minlen = (dmalen < -s->ti_size) ? dmalen : -s->ti_size;
> else
> @@ -452,7 +455,7 @@ void esp_reg_write(ESPState *s, uint32_t saddr, uint64_t
> val)
> break;
> case ESP_FIFO:
> if (s->do_cmd) {
> - if (s->cmdlen < TI_BUFSZ) {
> + if (s->cmdlen < ESP_CMDBUF_SZ) {
> s->cmdbuf[s->cmdlen++] = val & 0xff;
> } else {
> trace_esp_error_fifo_overrun();
> diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h
> index 6c79527..d2c4886 100644
> --- a/include/hw/scsi/esp.h
> +++ b/include/hw/scsi/esp.h
> @@ -14,6 +14,7 @@ void esp_init(hwaddr espaddr, int it_shift,
>
> #define ESP_REGS 16
> #define TI_BUFSZ 16
> +#define ESP_CMDBUF_SZ 32
>
> typedef struct ESPState ESPState;
>
> @@ -31,7 +32,7 @@ struct ESPState {
> SCSIBus bus;
> SCSIDevice *current_dev;
> SCSIRequest *current_req;
> - uint8_t cmdbuf[TI_BUFSZ];
> + uint8_t cmdbuf[ESP_CMDBUF_SZ];
> uint32_t cmdlen;
> uint32_t do_cmd;
>
>