From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60250) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b5Yke-0002Nj-4L for qemu-devel@nongnu.org; Wed, 25 May 2016 09:21:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b5YkX-0006Vi-8Y for qemu-devel@nongnu.org; Wed, 25 May 2016 09:21:31 -0400 Received: from mx2.suse.de ([195.135.220.15]:55372) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b5YkW-0006VU-Ve for qemu-devel@nongnu.org; Wed, 25 May 2016 09:21:25 -0400 References: <1464172291-2856-1-git-send-email-ppandit@redhat.com> <1464172291-2856-4-git-send-email-ppandit@redhat.com> <57458A95.5010506@suse.de> From: Hannes Reinecke Message-ID: <5745A6D1.30401@suse.de> Date: Wed, 25 May 2016 15:21:21 +0200 MIME-Version: 1.0 In-Reply-To: <57458A95.5010506@suse.de> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable 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: Alexander Graf , P J P , Qemu Developers Cc: Paolo Bonzini , Li Qiang , Prasad J Pandit On 05/25/2016 01:20 PM, Alexander Graf wrote: > 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=3D2048]' 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 =3D le32_to_cpu(initq->pi_addr_lo); >> pa_hi =3D le32_to_cpu(initq->pi_addr_hi); >> s->producer_pa =3D ((uint64_t) pa_hi << 32) | pa_lo; >> - s->reply_queue_head =3D ldl_le_pci_dma(pcid, s->producer_pa); >> - s->reply_queue_tail =3D ldl_le_pci_dma(pcid, s->consumer_pa); >> + s->reply_queue_head =3D \ >> + ldl_le_pci_dma(pcid, s->producer_pa) % MEGASAS_MAX_FRAMES; >> + s->reply_queue_tail =3D \ >> + ldl_le_pci_dma(pcid, s->consumer_pa) % MEGASAS_MAX_FRAMES; >=20 > You're using up 2 lines for each of those assignments now anyway, so > why not just split it into >=20 > s->reply_queue_head =3D ldl_le_pci_dma(pcid, s->producer_pa); > s->reply_queue_head %=3D MEGASAS_MAX_FRAMES; >=20 > and that way avoid a real multi-line statement (which is hard to read). >=20 > Whether the bitmask really is what hardware would do, I don't know. > I'll let Hannes comment there. Maybe the hardware simply does >=20 > s->reply_queue_head =3D MIN(ldl_le_pci_dma(pcid, s->producer_pa), > MEGASAS_MAX_FRAMES); >=20 > But I agree that the mask is more likely. >=20 ->reply_queue_head (and, consequently, ->producer_pa) is a ring buffer, which are supposed to wrap at MEGASAS_MAX_FRAMES. So 'MIN' is wrong; we need the mask as we _want_ the overflow to happen. Cheers, Hannes --=20 Dr. Hannes Reinecke Teamlead Storage & Networking hare@suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N=FCrnberg GF: F. Imend=F6rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG N=FCrnberg)