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;
-- 
2.25.4

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