On Thu, 7 May 2020 at 12:03, P J P <ppan...@redhat.com> wrote:
>
> From: Prasad J Pandit <p...@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 <rd...@gatech.edu>
> Signed-off-by: Prasad J Pandit <p...@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
