From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34509) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bD9fN-0004rQ-97 for qemu-devel@nongnu.org; Wed, 15 Jun 2016 08:11:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bD9fJ-0001Pd-WF for qemu-devel@nongnu.org; Wed, 15 Jun 2016 08:11:29 -0400 Received: from mx1.redhat.com ([209.132.183.28]:18074) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bD9fJ-0001PQ-Q7 for qemu-devel@nongnu.org; Wed, 15 Jun 2016 08:11:25 -0400 References: <1465982948-14639-1-git-send-email-ppandit@redhat.com> From: Laszlo Ersek Message-ID: <7cdb718e-6551-bf3a-a431-575aa29ac767@redhat.com> Date: Wed, 15 Jun 2016 14:11:22 +0200 MIME-Version: 1.0 In-Reply-To: <1465982948-14639-1-git-send-email-ppandit@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] scsi: esp: check length before dma read List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: P J P , Qemu Developers Cc: Paolo Bonzini , Prasad J Pandit , Li Qiang On 06/15/16 11:29, P J P wrote: > From: Prasad J Pandit > > 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 > Signed-off-by: Prasad J Pandit > --- > 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