From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56370) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b5Ws1-0003io-M6 for qemu-devel@nongnu.org; Wed, 25 May 2016 07:21:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b5Wrv-00079w-NX for qemu-devel@nongnu.org; Wed, 25 May 2016 07:21:00 -0400 Received: from mx2.suse.de ([195.135.220.15]:33465) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b5Wrv-00079r-GX for qemu-devel@nongnu.org; Wed, 25 May 2016 07:20:55 -0400 References: <1464172291-2856-1-git-send-email-ppandit@redhat.com> <1464172291-2856-4-git-send-email-ppandit@redhat.com> From: Alexander Graf Message-ID: <57458A95.5010506@suse.de> Date: Wed, 25 May 2016 13:20:53 +0200 MIME-Version: 1.0 In-Reply-To: <1464172291-2856-4-git-send-email-ppandit@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/3] scsi: megasas: check 'read_queue_head' index value List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: P J P , Qemu Developers Cc: Paolo Bonzini , Li Qiang , Hannes Reinecke , Prasad J Pandit On 05/25/2016 12:31 PM, P J P wrote: > From: Prasad J Pandit > > While doing MegaRAID SAS controller command frame lookup, routine > 'megasas_lookup_frame' uses 'read_queue_head' value as an index > into 'frames[MEGASAS_MAX_FRAMES=2048]' array. Limit its value > within array bounds to avoid any OOB access. > > Reported-by: Li Qiang > Signed-off-by: Prasad J Pandit > --- > hw/scsi/megasas.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c > index 7c08932..76d57f9 100644 > --- a/hw/scsi/megasas.c > +++ b/hw/scsi/megasas.c > @@ -649,8 +649,10 @@ static int megasas_init_firmware(MegasasState *s, MegasasCmd *cmd) > pa_lo = le32_to_cpu(initq->pi_addr_lo); > pa_hi = le32_to_cpu(initq->pi_addr_hi); > s->producer_pa = ((uint64_t) pa_hi << 32) | pa_lo; > - s->reply_queue_head = ldl_le_pci_dma(pcid, s->producer_pa); > - s->reply_queue_tail = ldl_le_pci_dma(pcid, s->consumer_pa); > + s->reply_queue_head = \ > + ldl_le_pci_dma(pcid, s->producer_pa) % MEGASAS_MAX_FRAMES; > + s->reply_queue_tail = \ > + ldl_le_pci_dma(pcid, s->consumer_pa) % MEGASAS_MAX_FRAMES; You're using up 2 lines for each of those assignments now anyway, so why not just split it into s->reply_queue_head = ldl_le_pci_dma(pcid, s->producer_pa); s->reply_queue_head %= MEGASAS_MAX_FRAMES; and that way avoid a real multi-line statement (which is hard to read). Whether the bitmask really is what hardware would do, I don't know. I'll let Hannes comment there. Maybe the hardware simply does s->reply_queue_head = MIN(ldl_le_pci_dma(pcid, s->producer_pa), MEGASAS_MAX_FRAMES); But I agree that the mask is more likely. Alex