Patchwork ppc405_uc: Fix buffer overflow

login
register
mail settings
Submitter Stefan Weil
Date Aug. 31, 2012, 8:21 p.m.
Message ID <1346444481-31727-1-git-send-email-sw@weilnetz.de>
Download mbox | patch
Permalink /patch/181052/
State Accepted
Headers show

Comments

Stefan Weil - Aug. 31, 2012, 8:21 p.m.
Report from smatch:

ppc405_uc.c:209 dcr_read_pob(12) error: buffer overflow 'pob->besr' 2 <= 2
ppc405_uc.c:232 dcr_write_pob(12) error: buffer overflow 'pob->besr' 2 <= 2

The old code reads and writes besr[POB0_BESR1 - POB0_BESR0] or besr[2]
which is one too much.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---

As this code was wrong for more than 5 years, there is no urgent need to
fix it now for QEMU 1.2.

Regards,

Stefan Weil

 hw/ppc405_uc.c |   16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)
Andreas Färber - Aug. 31, 2012, 9:36 p.m.
Am 31.08.2012 22:21, schrieb Stefan Weil:
> Report from smatch:
> 
> ppc405_uc.c:209 dcr_read_pob(12) error: buffer overflow 'pob->besr' 2 <= 2
> ppc405_uc.c:232 dcr_write_pob(12) error: buffer overflow 'pob->besr' 2 <= 2
> 
> The old code reads and writes besr[POB0_BESR1 - POB0_BESR0] or besr[2]
> which is one too much.
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
> 
> As this code was wrong for more than 5 years, there is no urgent need to
> fix it now for QEMU 1.2.
> 
> Regards,
> 
> Stefan Weil
> 
>  hw/ppc405_uc.c |   16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ppc405_uc.c b/hw/ppc405_uc.c
> index 89e5013..b52ab2f 100644
> --- a/hw/ppc405_uc.c
> +++ b/hw/ppc405_uc.c
> @@ -191,7 +191,8 @@ enum {
>  typedef struct ppc4xx_pob_t ppc4xx_pob_t;
>  struct ppc4xx_pob_t {
>      uint32_t bear;
> -    uint32_t besr[2];
> +    uint32_t besr0;
> +    uint32_t besr1;
>  };
>  
>  static uint32_t dcr_read_pob (void *opaque, int dcrn)

Reviewed-by: Andreas Färber <afaerber@suse.de>

We could alternatively leave besr[2] and access it with hardcoded 0..1.

Adding qemu-stable to the mix so it can be backported to stable-1.2
after the release.

Andreas

> @@ -205,8 +206,10 @@ static uint32_t dcr_read_pob (void *opaque, int dcrn)
>          ret = pob->bear;
>          break;
>      case POB0_BESR0:
> +        ret = pob->besr0;
> +        break;
>      case POB0_BESR1:
> -        ret = pob->besr[dcrn - POB0_BESR0];
> +        ret = pob->besr1;
>          break;
>      default:
>          /* Avoid gcc warning */
> @@ -227,9 +230,12 @@ static void dcr_write_pob (void *opaque, int dcrn, uint32_t val)
>          /* Read only */
>          break;
>      case POB0_BESR0:
> +        /* Write-clear */
> +        pob->besr0 &= ~val;
> +        break;
>      case POB0_BESR1:
>          /* Write-clear */
> -        pob->besr[dcrn - POB0_BESR0] &= ~val;
> +        pob->besr1 &= ~val;
>          break;
>      }
>  }
> @@ -241,8 +247,8 @@ static void ppc4xx_pob_reset (void *opaque)
>      pob = opaque;
>      /* No error */
>      pob->bear = 0x00000000;
> -    pob->besr[0] = 0x0000000;
> -    pob->besr[1] = 0x0000000;
> +    pob->besr0 = 0x0000000;
> +    pob->besr1 = 0x0000000;
>  }
>  
>  static void ppc4xx_pob_init(CPUPPCState *env)
>
Markus Armbruster - Sept. 1, 2012, 5:45 a.m.
Andreas Färber <afaerber@suse.de> writes:

> Am 31.08.2012 22:21, schrieb Stefan Weil:
>> Report from smatch:
>> 
>> ppc405_uc.c:209 dcr_read_pob(12) error: buffer overflow 'pob->besr' 2 <= 2
>> ppc405_uc.c:232 dcr_write_pob(12) error: buffer overflow 'pob->besr' 2 <= 2
>> 
>> The old code reads and writes besr[POB0_BESR1 - POB0_BESR0] or besr[2]
>> which is one too much.
>> 
>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>> ---
>> 
>> As this code was wrong for more than 5 years, there is no urgent need to
>> fix it now for QEMU 1.2.
>> 
>> Regards,
>> 
>> Stefan Weil
>> 
>>  hw/ppc405_uc.c |   16 +++++++++++-----
>>  1 file changed, 11 insertions(+), 5 deletions(-)
>> 
>> diff --git a/hw/ppc405_uc.c b/hw/ppc405_uc.c
>> index 89e5013..b52ab2f 100644
>> --- a/hw/ppc405_uc.c
>> +++ b/hw/ppc405_uc.c
>> @@ -191,7 +191,8 @@ enum {
>>  typedef struct ppc4xx_pob_t ppc4xx_pob_t;
>>  struct ppc4xx_pob_t {
>>      uint32_t bear;
>> -    uint32_t besr[2];
>> +    uint32_t besr0;
>> +    uint32_t besr1;
>>  };
>>  
>>  static uint32_t dcr_read_pob (void *opaque, int dcrn)
>
> Reviewed-by: Andreas Färber <afaerber@suse.de>
>
> We could alternatively leave besr[2] and access it with hardcoded 0..1.

Minimally invasive fix would be besr[dcrn != POB0_BESR0].

[...]
Alexander Graf - Sept. 1, 2012, 6:23 a.m.
On 31.08.2012, at 22:45, Markus Armbruster <armbru@redhat.com> wrote:

> Andreas Färber <afaerber@suse.de> writes:
> 
>> Am 31.08.2012 22:21, schrieb Stefan Weil:
>>> Report from smatch:
>>> 
>>> ppc405_uc.c:209 dcr_read_pob(12) error: buffer overflow 'pob->besr' 2 <= 2
>>> ppc405_uc.c:232 dcr_write_pob(12) error: buffer overflow 'pob->besr' 2 <= 2
>>> 
>>> The old code reads and writes besr[POB0_BESR1 - POB0_BESR0] or besr[2]
>>> which is one too much.
>>> 
>>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>>> ---
>>> 
>>> As this code was wrong for more than 5 years, there is no urgent need to
>>> fix it now for QEMU 1.2.
>>> 
>>> Regards,
>>> 
>>> Stefan Weil
>>> 
>>> hw/ppc405_uc.c |   16 +++++++++++-----
>>> 1 file changed, 11 insertions(+), 5 deletions(-)
>>> 
>>> diff --git a/hw/ppc405_uc.c b/hw/ppc405_uc.c
>>> index 89e5013..b52ab2f 100644
>>> --- a/hw/ppc405_uc.c
>>> +++ b/hw/ppc405_uc.c
>>> @@ -191,7 +191,8 @@ enum {
>>> typedef struct ppc4xx_pob_t ppc4xx_pob_t;
>>> struct ppc4xx_pob_t {
>>>     uint32_t bear;
>>> -    uint32_t besr[2];
>>> +    uint32_t besr0;
>>> +    uint32_t besr1;
>>> };
>>> 
>>> static uint32_t dcr_read_pob (void *opaque, int dcrn)
>> 
>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>> 
>> We could alternatively leave besr[2] and access it with hardcoded 0..1.
> 
> Minimally invasive fix would be besr[dcrn != POB0_BESR0].
> 
> [...]

I don't think the change is important enough for these stylistic questions :). I'll just apply it once I'm back to a real internet connection.

Alex
Stefan Weil - Sept. 1, 2012, 6:49 a.m.
Am 01.09.2012 08:23, schrieb Alexander Graf:
>
>
> On 31.08.2012, at 22:45, Markus Armbruster <armbru@redhat.com> wrote:
>
>> Andreas Färber <afaerber@suse.de> writes:
>> static uint32_t dcr_read_pob (void *opaque, int dcrn)

...

>>
>>>
>>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>>>
>>> We could alternatively leave besr[2] and access it with hardcoded 0..1.
>>
>> Minimally invasive fix would be besr[dcrn != POB0_BESR0].
>>
>> [...]
>
> I don't think the change is important enough for these stylistic questions :). I'll just apply it once I'm back to a real internet connection.
>
> Alex

Of course I considered those minimally invasive solutions.

There was already other code in the same file which used besr0, besr1,
and the wrong statements were simple enough to justify a duplication.
If I were a compiler, I'd generate smaller and faster code with the
new code :-)

Cheers,
Stefan W.
Alexander Graf - Sept. 20, 2012, 9:33 a.m.
On 31.08.2012, at 22:21, Stefan Weil wrote:

> Report from smatch:
> 
> ppc405_uc.c:209 dcr_read_pob(12) error: buffer overflow 'pob->besr' 2 <= 2
> ppc405_uc.c:232 dcr_write_pob(12) error: buffer overflow 'pob->besr' 2 <= 2
> 
> The old code reads and writes besr[POB0_BESR1 - POB0_BESR0] or besr[2]
> which is one too much.
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>

Thanks, applied to ppc-next.


Alex

Patch

diff --git a/hw/ppc405_uc.c b/hw/ppc405_uc.c
index 89e5013..b52ab2f 100644
--- a/hw/ppc405_uc.c
+++ b/hw/ppc405_uc.c
@@ -191,7 +191,8 @@  enum {
 typedef struct ppc4xx_pob_t ppc4xx_pob_t;
 struct ppc4xx_pob_t {
     uint32_t bear;
-    uint32_t besr[2];
+    uint32_t besr0;
+    uint32_t besr1;
 };
 
 static uint32_t dcr_read_pob (void *opaque, int dcrn)
@@ -205,8 +206,10 @@  static uint32_t dcr_read_pob (void *opaque, int dcrn)
         ret = pob->bear;
         break;
     case POB0_BESR0:
+        ret = pob->besr0;
+        break;
     case POB0_BESR1:
-        ret = pob->besr[dcrn - POB0_BESR0];
+        ret = pob->besr1;
         break;
     default:
         /* Avoid gcc warning */
@@ -227,9 +230,12 @@  static void dcr_write_pob (void *opaque, int dcrn, uint32_t val)
         /* Read only */
         break;
     case POB0_BESR0:
+        /* Write-clear */
+        pob->besr0 &= ~val;
+        break;
     case POB0_BESR1:
         /* Write-clear */
-        pob->besr[dcrn - POB0_BESR0] &= ~val;
+        pob->besr1 &= ~val;
         break;
     }
 }
@@ -241,8 +247,8 @@  static void ppc4xx_pob_reset (void *opaque)
     pob = opaque;
     /* No error */
     pob->bear = 0x00000000;
-    pob->besr[0] = 0x0000000;
-    pob->besr[1] = 0x0000000;
+    pob->besr0 = 0x0000000;
+    pob->besr1 = 0x0000000;
 }
 
 static void ppc4xx_pob_init(CPUPPCState *env)