| Message ID | 20200507105718.1319187-2-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | use unsigned type for MegasasState fields | expand |
On Thu, 7 May 2020 at 12:03, P J P <ppandit@redhat.com> wrote: > > From: Prasad J Pandit <pjp@fedoraproject.org> > > A guest user may set 's->reply_queue_head' MegasasState field to > a negative value. Later in 'megasas_lookup_frame' it is used to > index into s->frames[] array. Use unsigned type to avoid OOB > access issue. > > Reported-by: Ren Ding <rding@gatech.edu> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > --- > hw/scsi/megasas.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c > index af18c88b65..433353bad0 100644 > --- a/hw/scsi/megasas.c > +++ b/hw/scsi/megasas.c > @@ -112,7 +112,7 @@ typedef struct MegasasState { > uint64_t reply_queue_pa; > void *reply_queue; > int reply_queue_len; > - int reply_queue_head; > + uint16_t reply_queue_head; > int reply_queue_tail; > uint64_t consumer_pa; > uint64_t producer_pa; Using a 16-bit type here means that code like this: s->reply_queue_head = ldl_le_pci_dma(pcid, s->producer_pa); s->reply_queue_head %= MEGASAS_MAX_FRAMES; suddenly does a truncation of the 32-bit loaded value before the modulus operation, which is a bit odd, though since MEGASAS_MAX_FRAMES happens to be a power of 2 the end result won't change. It's also a bit weird to change reply_queue_head's type but not reply_queue_tail or reply_queue_len. thanks -- PMM
+-- On Tue, 12 May 2020, Peter Maydell wrote --+ | > + uint16_t reply_queue_head; | | Using a 16-bit type here means that code like this: | | s->reply_queue_head = ldl_le_pci_dma(pcid, s->producer_pa); | s->reply_queue_head %= MEGASAS_MAX_FRAMES; | | suddenly does a truncation of the 32-bit loaded value before | the modulus operation, which is a bit odd, though since | MEGASAS_MAX_FRAMES happens to be a power of 2 the end | result won't change. Yes, 16-bit also for its range of value is limitted to MEGASAS_MAX_FRAMES=2048. | It's also a bit weird to change reply_queue_head's type | but not reply_queue_tail or reply_queue_len. That's in the second patch. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c index af18c88b65..433353bad0 100644 --- a/hw/scsi/megasas.c +++ b/hw/scsi/megasas.c @@ -112,7 +112,7 @@ typedef struct MegasasState { uint64_t reply_queue_pa; void *reply_queue; int reply_queue_len; - int reply_queue_head; + uint16_t reply_queue_head; int reply_queue_tail; uint64_t consumer_pa; uint64_t producer_pa;