* [Qemu-devel] [PATCH] ppc405_uc: Fix buffer overflow
@ 2012-08-31 20:21 Stefan Weil
2012-08-31 21:36 ` Andreas Färber
2012-09-20 9:33 ` Alexander Graf
0 siblings, 2 replies; 6+ messages in thread
From: Stefan Weil @ 2012-08-31 20:21 UTC (permalink / raw)
To: Alexander Graf; +Cc: qemu-trivial, Stefan Weil, qemu-devel
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)
@@ -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)
--
1.7.10
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] ppc405_uc: Fix buffer overflow
2012-08-31 20:21 [Qemu-devel] [PATCH] ppc405_uc: Fix buffer overflow Stefan Weil
@ 2012-08-31 21:36 ` Andreas Färber
2012-09-01 5:45 ` Markus Armbruster
2012-09-20 9:33 ` Alexander Graf
1 sibling, 1 reply; 6+ messages in thread
From: Andreas Färber @ 2012-08-31 21:36 UTC (permalink / raw)
To: Stefan Weil; +Cc: qemu-trivial, qemu-stable, Alexander Graf, qemu-devel
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)
>
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] ppc405_uc: Fix buffer overflow
2012-08-31 21:36 ` Andreas Färber
@ 2012-09-01 5:45 ` Markus Armbruster
2012-09-01 6:23 ` Alexander Graf
0 siblings, 1 reply; 6+ messages in thread
From: Markus Armbruster @ 2012-09-01 5:45 UTC (permalink / raw)
To: Andreas Färber
Cc: qemu-trivial, Stefan Weil, qemu-devel, qemu-stable,
Alexander Graf
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].
[...]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] ppc405_uc: Fix buffer overflow
2012-09-01 5:45 ` Markus Armbruster
@ 2012-09-01 6:23 ` Alexander Graf
2012-09-01 6:49 ` Stefan Weil
0 siblings, 1 reply; 6+ messages in thread
From: Alexander Graf @ 2012-09-01 6:23 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-trivial@nongnu.org, Stefan Weil, qemu-devel@nongnu.org,
Andreas Färber, qemu-stable@nongnu.org
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] ppc405_uc: Fix buffer overflow
2012-09-01 6:23 ` Alexander Graf
@ 2012-09-01 6:49 ` Stefan Weil
0 siblings, 0 replies; 6+ messages in thread
From: Stefan Weil @ 2012-09-01 6:49 UTC (permalink / raw)
To: Alexander Graf
Cc: qemu-trivial@nongnu.org, qemu-stable@nongnu.org,
Markus Armbruster, Andreas Färber, qemu-devel@nongnu.org
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] ppc405_uc: Fix buffer overflow
2012-08-31 20:21 [Qemu-devel] [PATCH] ppc405_uc: Fix buffer overflow Stefan Weil
2012-08-31 21:36 ` Andreas Färber
@ 2012-09-20 9:33 ` Alexander Graf
1 sibling, 0 replies; 6+ messages in thread
From: Alexander Graf @ 2012-09-20 9:33 UTC (permalink / raw)
To: Stefan Weil; +Cc: qemu-trivial, qemu-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-09-20 9:34 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-08-31 20:21 [Qemu-devel] [PATCH] ppc405_uc: Fix buffer overflow Stefan Weil
2012-08-31 21:36 ` Andreas Färber
2012-09-01 5:45 ` Markus Armbruster
2012-09-01 6:23 ` Alexander Graf
2012-09-01 6:49 ` Stefan Weil
2012-09-20 9:33 ` Alexander Graf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).