[Qemu-devel] [PATCH 0/3] Qemu: scsi: megasas: various OOB r/w access checks

[Qemu-devel] [PATCH 0/3] Qemu: scsi: megasas: various OOB r/w access checks

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

Hello,

Multiple OOB r/w access issues were reported by Li Qiang, in SCSI MegaRAID SAS
Host bus Adapter emulation. Please see below proposed patches to fix these
issues.

Thank you.
--
Prasad J Pandit (3):
  scsi: megasas: use appropriate property buffer size
  scsi: megasas: initialise local configuration data buffer
  scsi: megasas: check 'read_queue_head' index value

 hw/scsi/megasas.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

-- 
2.5.5

[Qemu-devel] [PATCH 1/3] scsi: megasas: use appropriate property buffer size

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

When setting MegaRAID SAS controller properties via MegaRAID
Firmware Interface(MFI) commands, a user supplied size parameter
is used to set property value. Use appropriate size value to avoid
OOB access issues.

Reported-by: Li Qiang <liqiang6-s@360.cn>
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 a63a581..dcbd3e1 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -1446,7 +1446,7 @@ static int megasas_dcmd_set_properties(MegasasState *s, MegasasCmd *cmd)
                                             dcmd_size);
         return MFI_STAT_INVALID_PARAMETER;
     }
-    dma_buf_write((uint8_t *)&info, cmd->iov_size, &cmd->qsg);
+    dma_buf_write((uint8_t *)&info, dcmd_size, &cmd->qsg);
     trace_megasas_dcmd_unsupported(cmd->index, cmd->iov_size);
     return MFI_STAT_OK;
 }
-- 
2.5.5

[Qemu-devel] [PATCH 2/3] scsi: megasas: initialise local configuration data buffer

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

When reading MegaRAID SAS controller configuration via MegaRAID
Firmware Interface(MFI) commands, routine megasas_dcmd_cfg_read
uses an uninitialised local data buffer. Initialise this buffer
to avoid stack information leakage.

Reported-by: Li Qiang <liqiang6-s@360.cn>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/scsi/megasas.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index dcbd3e1..7c08932 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -1305,6 +1305,7 @@ static int megasas_dcmd_cfg_read(MegasasState *s, MegasasCmd *cmd)
     QTAILQ_FOREACH(kid, &s->bus.qbus.children, sibling) {
         num_pd_disks++;
     }
+    memset(data, 0, sizeof(data));
     info = (struct mfi_config_data *)&data;
     /*
      * Array mapping:
-- 
2.5.5

[Qemu-devel] [PATCH 3/3] scsi: megasas: check 'read_queue_head' index value

[P J P]

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;
     flags = le32_to_cpu(initq->flags);
     if (flags & MFI_QUEUE_FLAG_CONTEXT64) {
         s->flags |= MEGASAS_MASK_USE_QUEUE64;
-- 
2.5.5

Re: [Qemu-devel] [PATCH 1/3] scsi: megasas: use appropriate property buffer size

[Alexander Graf]

On 05/25/2016 12:31 PM, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> When setting MegaRAID SAS controller properties via MegaRAID
> Firmware Interface(MFI) commands, a user supplied size parameter
> is used to set property value. Use appropriate size value to avoid
> OOB access issues.
>
> Reported-by: Li Qiang <liqiang6-s@360.cn>
> 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 a63a581..dcbd3e1 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -1446,7 +1446,7 @@ static int megasas_dcmd_set_properties(MegasasState *s, MegasasCmd *cmd)
>                                               dcmd_size);
>           return MFI_STAT_INVALID_PARAMETER;
>       }
> -    dma_buf_write((uint8_t *)&info, cmd->iov_size, &cmd->qsg);
> +    dma_buf_write((uint8_t *)&info, dcmd_size, &cmd->qsg);

This looks odd - can dcmd_size be bigger than iov_size? Wouldn't we 
overwrite guest memory then? And where does dcmd_size come from? I don't 
see it in master.


Alex

>       trace_megasas_dcmd_unsupported(cmd->index, cmd->iov_size);
>       return MFI_STAT_OK;
>   }

Re: [Qemu-devel] [PATCH 2/3] scsi: megasas: initialise local configuration data buffer

[Alexander Graf]

On 05/25/2016 12:31 PM, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> When reading MegaRAID SAS controller configuration via MegaRAID
> Firmware Interface(MFI) commands, routine megasas_dcmd_cfg_read
> uses an uninitialised local data buffer. Initialise this buffer
> to avoid stack information leakage.
>
> Reported-by: Li Qiang <liqiang6-s@360.cn>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>   hw/scsi/megasas.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index dcbd3e1..7c08932 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -1305,6 +1305,7 @@ static int megasas_dcmd_cfg_read(MegasasState *s, MegasasCmd *cmd)
>       QTAILQ_FOREACH(kid, &s->bus.qbus.children, sibling) {
>           num_pd_disks++;
>       }
> +    memset(data, 0, sizeof(data));

Just replace the variable declaration with

   uint8_t data[4096] = "";

or

   uint8_t data[4096] = { 0 };

That should automatically get you a fully zeroed frame on the stack.


Alex

>       info = (struct mfi_config_data *)&data;
>       /*
>        * Array mapping:

Re: [Qemu-devel] [PATCH 3/3] scsi: megasas: check 'read_queue_head' index value

[Alexander Graf]

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

Re: [Qemu-devel] [PATCH 1/3] scsi: megasas: use appropriate property buffer size

[P J P]

  Hello Alex,

+-- On Wed, 25 May 2016, Alexander Graf wrote --+
| > -    dma_buf_write((uint8_t *)&info, cmd->iov_size, &cmd->qsg);
| > +    dma_buf_write((uint8_t *)&info, dcmd_size, &cmd->qsg);
| 
| This looks odd - can dcmd_size be bigger than iov_size? Wouldn't we overwrite
| guest memory then? And where does dcmd_size come from? I don't see it in
| master.

  struct mfi_ctrl_props info;
  size_t dcmd_size = sizeof(info);
  
  -> http://git.qemu.org/?p=qemu.git;a=blob;f=hw/scsi/megasas.c;h=a63a581550a328d0326ddee4f7fe1c4ffdecc194;hb=HEAD#l1439

'dcmd_size' is same as that of 'info' object.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH 1/3] scsi: megasas: use appropriate property buffer size

[Alexander Graf]

On 05/25/2016 01:51 PM, P J P wrote:
>    Hello Alex,
>
> +-- On Wed, 25 May 2016, Alexander Graf wrote --+
> | > -    dma_buf_write((uint8_t *)&info, cmd->iov_size, &cmd->qsg);
> | > +    dma_buf_write((uint8_t *)&info, dcmd_size, &cmd->qsg);
> |
> | This looks odd - can dcmd_size be bigger than iov_size? Wouldn't we overwrite
> | guest memory then? And where does dcmd_size come from? I don't see it in
> | master.
>
>    struct mfi_ctrl_props info;
>    size_t dcmd_size = sizeof(info);
>    
>    -> http://git.qemu.org/?p=qemu.git;a=blob;f=hw/scsi/megasas.c;h=a63a581550a328d0326ddee4f7fe1c4ffdecc194;hb=HEAD#l1439
>
> 'dcmd_size' is same as that of 'info' object.

Ok, then this patch is definitely bogus. The guest may receive less than 
the size of the info object. So we really want to have a MIN() between 
the maximum allowed transfer size (sizeof(info)) and the requested size 
(cmd->iov_size) here.

Alex

Re: [Qemu-devel] [PATCH 0/3] Qemu: scsi: megasas: various OOB r/w access checks

[Paolo Bonzini]

On 25/05/2016 12:31, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> Hello,
> 
> Multiple OOB r/w access issues were reported by Li Qiang, in SCSI MegaRAID SAS
> Host bus Adapter emulation. Please see below proposed patches to fix these
> issues.

Thanks, queued.

Paolo

Re: [Qemu-devel] [PATCH 0/3] Qemu: scsi: megasas: various OOB r/w access checks

[Alexander Graf]

On 05/25/2016 02:00 PM, Paolo Bonzini wrote:
>
> On 25/05/2016 12:31, P J P wrote:
>> From: Prasad J Pandit <pjp@fedoraproject.org>
>>
>> Hello,
>>
>> Multiple OOB r/w access issues were reported by Li Qiang, in SCSI MegaRAID SAS
>> Host bus Adapter emulation. Please see below proposed patches to fix these
>> issues.
> Thanks, queued.

Not sure that's a good idea with review comments on all patches ;)

Alex

Re: [Qemu-devel] [PATCH 1/3] scsi: megasas: use appropriate property buffer size

[P J P]

+-- On Wed, 25 May 2016, Alexander Graf wrote --+
| >    http://git.qemu.org/?p=qemu.git;a=blob;f=hw/scsi/megasas.c;h=a63a581550a328d0326ddee4f7fe1c4ffdecc194;hb=HEAD#l1439
| > 'dcmd_size' is same as that of 'info' object.
| 
| Ok, then this patch is definitely bogus. The guest may receive less than the
| size of the info object. So we really want to have a MIN() between the maximum
| allowed transfer size (sizeof(info)) and the requested size (cmd->iov_size)
| here.

There is also a check which returns an invalid parameter error if 
'cmd->iov_size' is less than 'dcmd_size'. Ie. OOB access occurs when 
cmd->iov_size is greater than 'dcmd_size'.

--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH 1/3] scsi: megasas: use appropriate property buffer size

[Alexander Graf]

On 05/25/2016 02:15 PM, P J P wrote:
> +-- On Wed, 25 May 2016, Alexander Graf wrote --+
> | >    http://git.qemu.org/?p=qemu.git;a=blob;f=hw/scsi/megasas.c;h=a63a581550a328d0326ddee4f7fe1c4ffdecc194;hb=HEAD#l1439
> | > 'dcmd_size' is same as that of 'info' object.
> |
> | Ok, then this patch is definitely bogus. The guest may receive less than the
> | size of the info object. So we really want to have a MIN() between the maximum
> | allowed transfer size (sizeof(info)) and the requested size (cmd->iov_size)
> | here.
>
> There is also a check which returns an invalid parameter error if
> 'cmd->iov_size' is less than 'dcmd_size'. Ie. OOB access occurs when
> cmd->iov_size is greater than 'dcmd_size'.

Turns out you're much better at reading code than me - yes, true, all is 
good :)

Reviewed-by: Alexander Graf <agraf@suse.de>

Alex

Re: [Qemu-devel] [PATCH 3/3] scsi: megasas: check 'read_queue_head' index value

[Hannes Reinecke]

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
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)