Re: [PATCH 1/2] megasas: use unsigned type for reply_queue_head

[Peter Maydell]

On Thu, 7 May 2020 at 12:03, P J P <address@hidden> wrote:
>
> From: Prasad J Pandit <address@hidden>
>
> 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 <address@hidden>
> Signed-off-by: Prasad J Pandit <address@hidden>
> ---
>  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