| Message ID | 1464172291-2856-4-git-send-email-ppandit@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 05/25/2016 12:31 PM, P J P wrote: > From: Prasad J Pandit <pjp@fedoraproject.org> > > 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 <liqiang6-s@360.cn> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > --- > 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
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 <pjp@fedoraproject.org> >> >> 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 <liqiang6-s@360.cn> >> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> >> --- >> 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. > ->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
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; flags = le32_to_cpu(initq->flags); if (flags & MFI_QUEUE_FLAG_CONTEXT64) { s->flags |= MEGASAS_MASK_USE_QUEUE64;