* [Qemu-devel] [PATCH v3] scsi: esp: check length before dma read
@ 2016-06-15 16:16 P J P
2016-06-15 16:34 ` Paolo Bonzini
2016-06-15 16:36 ` Paolo Bonzini
0 siblings, 2 replies; 5+ messages in thread
From: P J P @ 2016-06-15 16:16 UTC (permalink / raw)
To: Qemu Developers; +Cc: Paolo Bonzini, Li Qiang, Laszlo Ersek, Prasad J Pandit
From: Prasad J Pandit <pjp@fedoraproject.org>
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 <liqiang6-s@360.cn>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
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
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;
+ }
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;
--
2.5.5
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v3] scsi: esp: check length before dma read
2016-06-15 16:16 [Qemu-devel] [PATCH v3] scsi: esp: check length before dma read P J P
@ 2016-06-15 16:34 ` Paolo Bonzini
2016-06-15 16:36 ` Paolo Bonzini
1 sibling, 0 replies; 5+ messages in thread
From: Paolo Bonzini @ 2016-06-15 16:34 UTC (permalink / raw)
To: P J P, Qemu Developers; +Cc: Li Qiang, Laszlo Ersek, Prasad J Pandit
On 15/06/2016 18:16, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> 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 <liqiang6-s@360.cn>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
> 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;
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v3] scsi: esp: check length before dma read
2016-06-15 16:16 [Qemu-devel] [PATCH v3] scsi: esp: check length before dma read P J P
2016-06-15 16:34 ` Paolo Bonzini
@ 2016-06-15 16:36 ` Paolo Bonzini
2016-06-15 17:18 ` P J P
1 sibling, 1 reply; 5+ messages in thread
From: Paolo Bonzini @ 2016-06-15 16:36 UTC (permalink / raw)
To: P J P, Qemu Developers; +Cc: Li Qiang, Laszlo Ersek, Prasad J Pandit
On 15/06/2016 18:16, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> 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.
Actually, the commit message is wrong. The length parameter cannot
exceed the buffer size anymore. Can you do a v4 with the corrected
commit message and an assert that avoids overflows like in Laszlo's
proposal? I think this:
assert (s->cmdlen <= sizeof(s->cmdbuf) &&
len <= sizeof(s->cmdbuf) - s->cmdlen);
would do.
Thanks,
Paolo
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v3] scsi: esp: check length before dma read
2016-06-15 16:36 ` Paolo Bonzini
@ 2016-06-15 17:18 ` P J P
2016-06-15 18:27 ` Paolo Bonzini
0 siblings, 1 reply; 5+ messages in thread
From: P J P @ 2016-06-15 17:18 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Qemu Developers, Li Qiang, Laszlo Ersek
Hello Paolo,
+-- On Wed, 15 Jun 2016, Paolo Bonzini wrote --+
| Actually, the commit message is wrong. The length parameter cannot
| exceed the buffer size anymore.
It wouldn't exceed after this patch, right? Is it possible 'esp_do_dma' is
called via 'esp_transfer_data' with 's->do_cmd' set? 'len' isn't checked
there.
| Can you do a v4 with the corrected
| commit message and an assert that avoids overflows like in Laszlo's
| proposal? I think this:
|
| assert (s->cmdlen <= sizeof(s->cmdbuf) &&
| len <= sizeof(s->cmdbuf) - s->cmdlen);
Okay.
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v3] scsi: esp: check length before dma read
2016-06-15 17:18 ` P J P
@ 2016-06-15 18:27 ` Paolo Bonzini
0 siblings, 0 replies; 5+ messages in thread
From: Paolo Bonzini @ 2016-06-15 18:27 UTC (permalink / raw)
To: P J P; +Cc: Qemu Developers, Li Qiang, Laszlo Ersek
On 15/06/2016 19:18, P J P wrote:
> Hello Paolo,
>
> +-- On Wed, 15 Jun 2016, Paolo Bonzini wrote --+
> | Actually, the commit message is wrong. The length parameter cannot
> | exceed the buffer size anymore.
>
> It wouldn't exceed after this patch, right? Is it possible 'esp_do_dma' is
> called via 'esp_transfer_data' with 's->do_cmd' set? 'len' isn't checked
> there.
No, it's not possible; see the discussion in reply to v1.
Paolo
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-06-15 18:27 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-15 16:16 [Qemu-devel] [PATCH v3] scsi: esp: check length before dma read P J P
2016-06-15 16:34 ` Paolo Bonzini
2016-06-15 16:36 ` Paolo Bonzini
2016-06-15 17:18 ` P J P
2016-06-15 18:27 ` Paolo Bonzini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.