| Message ID | 1346444481-31727-1-git-send-email-sw@weilnetz.de |
|---|---|
| State | Accepted |
| Headers | show |
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) >
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]. [...]
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
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.
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
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)
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(-)