| Message ID | 1465982948-14639-1-git-send-email-ppandit@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 06/15/16 11:29, 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. > > Reported-by: Li Qiang <qiang6-s@360.cn> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > --- > hw/scsi/esp.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c > index 4b94bbc..dfea571 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 (s->cmdlen + len >= sizeof(s->cmdbuf)) { > + return; > + } > s->dma_memory_read(s->dma_opaque, &s->cmdbuf[s->cmdlen], len); > s->ti_size = 0; > s->cmdlen = 0; > (1) In my opinion, this check is not sufficient. All of the following objects: - the "len" local variable - the "ESPState.dma_left" field - the "ESPState.cmdlen" field have type "uint32_t" (that is, "unsigned int"). Therefore the addition on the LHS is performed in "unsigned int", resulting in (well-defined, but still harmful) wrapping at 2^32. (2) I think the check may be off-by-one. If (s->cmdlen + len) equal sizeof(s->cmdbuf), that should be allowed, shouldn't it? Then the dma_memory_read() function just after will access the cmd buffer right to its end, but not after. So, I suggest the following instead: if (s->cmdlen > sizeof(s->cmdbuf) || len > sizeof(s->cmdbuf) - s->cmdlen) { return; } The first subcondition may be constant false at this point, due to an earlier check somewhere else in the code; I'm not sure. If that's the case, then the first subcondition can be dropped. Either way, the first subcondition makes sure that the subtraction in the second subcondition will never underflow. And the second subcondition expresses (without a possibility for overflow) the actual check we're interested in. Thanks Laszlo
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index 4b94bbc..dfea571 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 (s->cmdlen + len >= sizeof(s->cmdbuf)) { + return; + } s->dma_memory_read(s->dma_opaque, &s->cmdbuf[s->cmdlen], len); s->ti_size = 0; s->cmdlen = 0;