[Qemu-devel] [PATCH] ppc/e500_pci: Fix an array overflow issue

[Qemu-devel] [PATCH] ppc/e500_pci: Fix an array overflow issue

[Liu Yu]

Signed-off-by: Liu Yu <yu.liu@freescale.com>
---
 hw/ppce500_pci.c |   26 ++++++++++++++++----------
 1 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
index 2db365d..3e24e85 100644
--- a/hw/ppce500_pci.c
+++ b/hw/ppce500_pci.c
@@ -108,15 +108,18 @@ static uint32_t pci_reg_read4(void *opaque, target_phys_addr_t addr)
 
     case PPCE500_PCI_IW3:
     case PPCE500_PCI_IW2:
-    case PPCE500_PCI_IW1:
+    case PPCE500_PCI_IW1: {
+        int idx = ((addr >> 5) & 0x3) - 1;
+
         switch (addr & 0xC) {
-        case PCI_PITAR: value = pci->pib[(addr >> 5) & 0x3].pitar; break;
-        case PCI_PIWBAR: value = pci->pib[(addr >> 5) & 0x3].piwbar; break;
-        case PCI_PIWBEAR: value = pci->pib[(addr >> 5) & 0x3].piwbear; break;
-        case PCI_PIWAR: value = pci->pib[(addr >> 5) & 0x3].piwar; break;
+        case PCI_PITAR: value = pci->pib[idx].pitar; break;
+        case PCI_PIWBAR: value = pci->pib[idx].piwbar; break;
+        case PCI_PIWBEAR: value = pci->pib[idx].piwbear; break;
+        case PCI_PIWAR: value = pci->pib[idx].piwar; break;
         default: break;
         };
         break;
+    }
 
     case PPCE500_PCI_GASKET_TIMR:
         value = pci->gasket_time;
@@ -164,15 +167,18 @@ static void pci_reg_write4(void *opaque, target_phys_addr_t addr,
 
     case PPCE500_PCI_IW3:
     case PPCE500_PCI_IW2:
-    case PPCE500_PCI_IW1:
+    case PPCE500_PCI_IW1: {
+        int idx = ((addr >> 5) & 0x3) - 1;
+
         switch (addr & 0xC) {
-        case PCI_PITAR: pci->pib[(addr >> 5) & 0x3].pitar = value; break;
-        case PCI_PIWBAR: pci->pib[(addr >> 5) & 0x3].piwbar = value; break;
-        case PCI_PIWBEAR: pci->pib[(addr >> 5) & 0x3].piwbear = value; break;
-        case PCI_PIWAR: pci->pib[(addr >> 5) & 0x3].piwar = value; break;
+        case PCI_PITAR: pci->pib[idx].pitar = value; break;
+        case PCI_PIWBAR: pci->pib[idx].piwbar = value; break;
+        case PCI_PIWBEAR: pci->pib[idx].piwbear = value; break;
+        case PCI_PIWAR: pci->pib[idx].piwar = value; break;
         default: break;
         };
         break;
+    }
 
     case PPCE500_PCI_GASKET_TIMR:
         pci->gasket_time = value;
-- 
1.6.4

Re: [Qemu-devel] [PATCH] ppc/e500_pci: Fix an array overflow issue

[Alexander Graf]

On 27.09.2011, at 10:17, Liu Yu wrote:

> Signed-off-by: Liu Yu <yu.liu@freescale.com>

Patch description missing.

Also, please always CC qemu-ppc@nongnu.org for patches concerning ppc.


> ---
> hw/ppce500_pci.c |   26 ++++++++++++++++----------
> 1 files changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
> index 2db365d..3e24e85 100644
> --- a/hw/ppce500_pci.c
> +++ b/hw/ppce500_pci.c
> @@ -108,15 +108,18 @@ static uint32_t pci_reg_read4(void *opaque, target_phys_addr_t addr)
> 
>     case PPCE500_PCI_IW3:
>     case PPCE500_PCI_IW2:
> -    case PPCE500_PCI_IW1:
> +    case PPCE500_PCI_IW1: {
> +        int idx = ((addr >> 5) & 0x3) - 1;

So this is the main change, right? Why the -1? A guest could potentially access pib[-1] using this, no?

> +
>         switch (addr & 0xC) {
> -        case PCI_PITAR: value = pci->pib[(addr >> 5) & 0x3].pitar; break;
> -        case PCI_PIWBAR: value = pci->pib[(addr >> 5) & 0x3].piwbar; break;
> -        case PCI_PIWBEAR: value = pci->pib[(addr >> 5) & 0x3].piwbear; break;
> -        case PCI_PIWAR: value = pci->pib[(addr >> 5) & 0x3].piwar; break;
> +        case PCI_PITAR: value = pci->pib[idx].pitar; break;
> +        case PCI_PIWBAR: value = pci->pib[idx].piwbar; break;
> +        case PCI_PIWBEAR: value = pci->pib[idx].piwbear; break;
> +        case PCI_PIWAR: value = pci->pib[idx].piwar; break;

I'm fairly sure this breaks checkpatch.pl.


Alex

Re: [Qemu-devel] [Qemu-ppc] [PATCH] ppc/e500_pci: Fix an array overflow issue

[Scott Wood]

On 09/27/2011 07:45 AM, Alexander Graf wrote:
> On 27.09.2011, at 10:17, Liu Yu wrote:
>> ---
>> hw/ppce500_pci.c |   26 ++++++++++++++++----------
>> 1 files changed, 16 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
>> index 2db365d..3e24e85 100644
>> --- a/hw/ppce500_pci.c
>> +++ b/hw/ppce500_pci.c
>> @@ -108,15 +108,18 @@ static uint32_t pci_reg_read4(void *opaque, target_phys_addr_t addr)
>>
>>     case PPCE500_PCI_IW3:
>>     case PPCE500_PCI_IW2:
>> -    case PPCE500_PCI_IW1:
>> +    case PPCE500_PCI_IW1: {
>> +        int idx = ((addr >> 5) & 0x3) - 1;
> 
> So this is the main change, right? Why the -1? A guest could potentially access pib[-1] using this, no?

Not with the values of addr that lead to this code.  The -1 is because
IW1/2/3 are 0x1e0/0x1c0/0x1a0.  Previously IW1 would overflow the array.

>>         switch (addr & 0xC) {
>> -        case PCI_PITAR: value = pci->pib[(addr >> 5) & 0x3].pitar; break;
>> -        case PCI_PIWBAR: value = pci->pib[(addr >> 5) & 0x3].piwbar; break;
>> -        case PCI_PIWBEAR: value = pci->pib[(addr >> 5) & 0x3].piwbear; break;
>> -        case PCI_PIWAR: value = pci->pib[(addr >> 5) & 0x3].piwar; break;
>> +        case PCI_PITAR: value = pci->pib[idx].pitar; break;
>> +        case PCI_PIWBAR: value = pci->pib[idx].piwbar; break;
>> +        case PCI_PIWBEAR: value = pci->pib[idx].piwbear; break;
>> +        case PCI_PIWAR: value = pci->pib[idx].piwar; break;
> 
> I'm fairly sure this breaks checkpatch.pl.

So does the original code...

If this is to be fixed, the outbound window switch should be fixed too
(and made to use idx, for consistency).

-Scott

Re: [Qemu-devel] [Qemu-ppc] [PATCH] ppc/e500_pci: Fix an array overflow issue

[Alexander Graf]

On 27.09.2011, at 18:52, Scott Wood wrote:

> On 09/27/2011 07:45 AM, Alexander Graf wrote:
>> On 27.09.2011, at 10:17, Liu Yu wrote:
>>> ---
>>> hw/ppce500_pci.c |   26 ++++++++++++++++----------
>>> 1 files changed, 16 insertions(+), 10 deletions(-)
>>> 
>>> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
>>> index 2db365d..3e24e85 100644
>>> --- a/hw/ppce500_pci.c
>>> +++ b/hw/ppce500_pci.c
>>> @@ -108,15 +108,18 @@ static uint32_t pci_reg_read4(void *opaque, target_phys_addr_t addr)
>>> 
>>>    case PPCE500_PCI_IW3:
>>>    case PPCE500_PCI_IW2:
>>> -    case PPCE500_PCI_IW1:
>>> +    case PPCE500_PCI_IW1: {
>>> +        int idx = ((addr >> 5) & 0x3) - 1;
>> 
>> So this is the main change, right? Why the -1? A guest could potentially access pib[-1] using this, no?
> 
> Not with the values of addr that lead to this code.  The -1 is because
> IW1/2/3 are 0x1e0/0x1c0/0x1a0.  Previously IW1 would overflow the array.

We're matching on addr & 0xfe0 and do the switch based on that. Possible values are:

  0x1a0
  0x1c0
  0x1e0

Then we >> 5 them.

  0xd
  0xe
  0xf

... and & 0x3 them

  0x1
  0x2
  0x0

... and apply -1:

  0x0
  0x1
  -1

> 
>>>        switch (addr & 0xC) {
>>> -        case PCI_PITAR: value = pci->pib[(addr >> 5) & 0x3].pitar; break;
>>> -        case PCI_PIWBAR: value = pci->pib[(addr >> 5) & 0x3].piwbar; break;
>>> -        case PCI_PIWBEAR: value = pci->pib[(addr >> 5) & 0x3].piwbear; break;
>>> -        case PCI_PIWAR: value = pci->pib[(addr >> 5) & 0x3].piwar; break;
>>> +        case PCI_PITAR: value = pci->pib[idx].pitar; break;
>>> +        case PCI_PIWBAR: value = pci->pib[idx].piwbar; break;
>>> +        case PCI_PIWBEAR: value = pci->pib[idx].piwbear; break;
>>> +        case PCI_PIWAR: value = pci->pib[idx].piwar; break;
>> 
>> I'm fairly sure this breaks checkpatch.pl.
> 
> So does the original code...
> 
> If this is to be fixed, the outbound window switch should be fixed too
> (and made to use idx, for consistency).

Yes, please. My preferred way to do this would be to send a cleanup patch that fixes the coding style issues first (can be in the same patch set) and then another patch for functional changes on top. Makes it easier to review.


Alex

Re: [Qemu-devel] [Qemu-ppc] [PATCH] ppc/e500_pci: Fix an array overflow issue

[Richard Henderson]

On 09/27/2011 10:01 AM, Alexander Graf wrote:
>   0xd
>   0xe
>   0xf
> 
> ... and & 0x3 them
> 
>   0x1
>   0x2
>   0x0

That's a remarkably different AND function...


r~

Re: [Qemu-devel] [Qemu-ppc] [PATCH] ppc/e500_pci: Fix an array overflow issue

[Alexander Graf]

On 27.09.2011, at 19:04, Richard Henderson wrote:

> On 09/27/2011 10:01 AM, Alexander Graf wrote:
>>  0xd
>>  0xe
>>  0xf
>> 
>> ... and & 0x3 them
>> 
>>  0x1
>>  0x2
>>  0x0
> 
> That's a remarkably different AND function...

No, it's a typo. I typed % instead of & and didn't realize it. Bleks.


:)


Alex

Re: [Qemu-devel] [Qemu-ppc] [PATCH] ppc/e500_pci: Fix an array overflow issue

[Scott Wood]

On 09/27/2011 12:01 PM, Alexander Graf wrote:
> 
> On 27.09.2011, at 18:52, Scott Wood wrote:
> 
>> On 09/27/2011 07:45 AM, Alexander Graf wrote:
>>> So this is the main change, right? Why the -1? A guest could potentially access pib[-1] using this, no?
>>
>> Not with the values of addr that lead to this code.  The -1 is because
>> IW1/2/3 are 0x1e0/0x1c0/0x1a0.  Previously IW1 would overflow the array.
> 
> We're matching on addr & 0xfe0 and do the switch based on that. Possible values are:
> 
>   0x1a0
>   0x1c0
>   0x1e0
> 
> Then we >> 5 them.
> 
>   0xd
>   0xe
>   0xf
> 
> ... and & 0x3 them
> 
>   0x1
>   0x2
>   0x0

0xd & 0x3 = 1
0xe & 0x3 = 2
0xf & 0x3 = 3

-Scott