* [PATCH 0/2] hw/timer/renesas_tmr: Fix use of uninitialized data @ 2021-02-19 22:32 Peter Maydell 2021-02-19 22:32 ` [PATCH 1/2] hw/timer/renesas_tmr: Prefix constants for CSS values with CSS_ Peter Maydell 2021-02-19 22:32 ` [PATCH 2/2] hw/timer/renesas_tmr: Fix use of uninitialized data in read_tcnt() Peter Maydell 0 siblings, 2 replies; 5+ messages in thread From: Peter Maydell @ 2021-02-19 22:32 UTC (permalink / raw) To: qemu-devel; +Cc: Magnus Damm, Yoshinori Sato This patchseries fixes a use-of-uninitialized-data spotted by Coverity (CID 1429976). Patch 1 just tweaks some constant names for values of the TCCR.CSS register field, since patch 2 needs to add some more defines for the other possible values of the field. Patch 2 is the bugfix proper; the use-uninitialized happens if the guest programs TCCR.CSS to values which are either prohibited in the h/w datasheet, or valid but corresponding to behaviour not currently implemented by QEMU. (Yes, I could have added LOG_UNIMP and/or LOG_GUEST_ERROR when the TCCR is written by the guest; it didn't really seem worth the effort to me.) thanks -- PMM Peter Maydell (2): hw/timer/renesas_tmr: Prefix constants for CSS values with CSS_ hw/timer/renesas_tmr: Fix use of uninitialized data in read_tcnt() hw/timer/renesas_tmr.c | 33 ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] hw/timer/renesas_tmr: Prefix constants for CSS values with CSS_ 2021-02-19 22:32 [PATCH 0/2] hw/timer/renesas_tmr: Fix use of uninitialized data Peter Maydell @ 2021-02-19 22:32 ` Peter Maydell 2021-02-19 23:10 ` Philippe Mathieu-Daudé 2021-02-19 22:32 ` [PATCH 2/2] hw/timer/renesas_tmr: Fix use of uninitialized data in read_tcnt() Peter Maydell 1 sibling, 1 reply; 5+ messages in thread From: Peter Maydell @ 2021-02-19 22:32 UTC (permalink / raw) To: qemu-devel; +Cc: Magnus Damm, Yoshinori Sato The #defines INTERNAL and CASCADING represent different possible values for the TCCR.CSS register field; prefix them with CSS_ to make this more obvious, before we add more defines to represent the other possible values of the field in the next commit. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/timer/renesas_tmr.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/hw/timer/renesas_tmr.c b/hw/timer/renesas_tmr.c index e03a8155b2b..22260aaaba5 100644 --- a/hw/timer/renesas_tmr.c +++ b/hw/timer/renesas_tmr.c @@ -46,8 +46,8 @@ REG8(TCCR, 10) FIELD(TCCR, CSS, 3, 2) FIELD(TCCR, TMRIS, 7, 1) -#define INTERNAL 0x01 -#define CASCADING 0x03 +#define CSS_INTERNAL 0x01 +#define CSS_CASCADING 0x03 #define CCLR_A 0x01 #define CCLR_B 0x02 @@ -72,7 +72,7 @@ static void update_events(RTMRState *tmr, int ch) /* event not happened */ return ; } - if (FIELD_EX8(tmr->tccr[0], TCCR, CSS) == CASCADING) { + if (FIELD_EX8(tmr->tccr[0], TCCR, CSS) == CSS_CASCADING) { /* cascading mode */ if (ch == 1) { tmr->next[ch] = none; @@ -130,7 +130,7 @@ static uint16_t read_tcnt(RTMRState *tmr, unsigned size, int ch) if (delta > 0) { tmr->tick = now; - if (FIELD_EX8(tmr->tccr[1], TCCR, CSS) == INTERNAL) { + if (FIELD_EX8(tmr->tccr[1], TCCR, CSS) == CSS_INTERNAL) { /* timer1 count update */ elapsed = elapsed_time(tmr, 1, delta); if (elapsed >= 0x100) { @@ -139,11 +139,11 @@ static uint16_t read_tcnt(RTMRState *tmr, unsigned size, int ch) tcnt[1] = tmr->tcnt[1] + (elapsed & 0xff); } switch (FIELD_EX8(tmr->tccr[0], TCCR, CSS)) { - case INTERNAL: + case CSS_INTERNAL: elapsed = elapsed_time(tmr, 0, delta); tcnt[0] = tmr->tcnt[0] + elapsed; break; - case CASCADING: + case CSS_CASCADING: if (ovf > 0) { tcnt[0] = tmr->tcnt[0] + ovf; } @@ -330,7 +330,7 @@ static uint16_t issue_event(RTMRState *tmr, int ch, int sz, qemu_irq_pulse(tmr->cmia[ch]); } if (sz == 8 && ch == 0 && - FIELD_EX8(tmr->tccr[1], TCCR, CSS) == CASCADING) { + FIELD_EX8(tmr->tccr[1], TCCR, CSS) == CSS_CASCADING) { tmr->tcnt[1]++; timer_events(tmr, 1); } @@ -362,7 +362,7 @@ static void timer_events(RTMRState *tmr, int ch) uint16_t tcnt; tmr->tcnt[ch] = read_tcnt(tmr, 1, ch); - if (FIELD_EX8(tmr->tccr[0], TCCR, CSS) != CASCADING) { + if (FIELD_EX8(tmr->tccr[0], TCCR, CSS) != CSS_CASCADING) { tmr->tcnt[ch] = issue_event(tmr, ch, 8, tmr->tcnt[ch], tmr->tcora[ch], -- 2.20.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] hw/timer/renesas_tmr: Prefix constants for CSS values with CSS_ 2021-02-19 22:32 ` [PATCH 1/2] hw/timer/renesas_tmr: Prefix constants for CSS values with CSS_ Peter Maydell @ 2021-02-19 23:10 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 5+ messages in thread From: Philippe Mathieu-Daudé @ 2021-02-19 23:10 UTC (permalink / raw) To: Peter Maydell, qemu-devel; +Cc: Magnus Damm, Yoshinori Sato On 2/19/21 11:32 PM, Peter Maydell wrote: > The #defines INTERNAL and CASCADING represent different possible > values for the TCCR.CSS register field; prefix them with CSS_ to make > this more obvious, before we add more defines to represent the > other possible values of the field in the next commit. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/timer/renesas_tmr.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 2/2] hw/timer/renesas_tmr: Fix use of uninitialized data in read_tcnt() 2021-02-19 22:32 [PATCH 0/2] hw/timer/renesas_tmr: Fix use of uninitialized data Peter Maydell 2021-02-19 22:32 ` [PATCH 1/2] hw/timer/renesas_tmr: Prefix constants for CSS values with CSS_ Peter Maydell @ 2021-02-19 22:32 ` Peter Maydell 2021-02-19 23:13 ` Philippe Mathieu-Daudé 1 sibling, 1 reply; 5+ messages in thread From: Peter Maydell @ 2021-02-19 22:32 UTC (permalink / raw) To: qemu-devel; +Cc: Magnus Damm, Yoshinori Sato The read_tcnt() function calculates the TCNT register values for the two channels of the timer module; it sets these up in the local tcnt[] array, and eventually returns either one or both of them, depending on whether the access is 8 or 16 bits. However, not all of the code paths through this function set both elements of this array: if the guest has programmed the TCCR.CSS register fields to values which are either documented as not to be used or which QEMU does not implement, then the function will return uninitialized data. (This was spotted by Coverity.) Add the missing CSS cases to this code, so that we return a consistent value instead of uninitialized data, and so the code structure indicates what's happening. Fixes: CID 1429976 Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/timer/renesas_tmr.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/hw/timer/renesas_tmr.c b/hw/timer/renesas_tmr.c index 22260aaaba5..eed39917fec 100644 --- a/hw/timer/renesas_tmr.c +++ b/hw/timer/renesas_tmr.c @@ -46,7 +46,9 @@ REG8(TCCR, 10) FIELD(TCCR, CSS, 3, 2) FIELD(TCCR, TMRIS, 7, 1) +#define CSS_EXTERNAL 0x00 #define CSS_INTERNAL 0x01 +#define CSS_INVALID 0x02 #define CSS_CASCADING 0x03 #define CCLR_A 0x01 #define CCLR_B 0x02 @@ -130,13 +132,20 @@ static uint16_t read_tcnt(RTMRState *tmr, unsigned size, int ch) if (delta > 0) { tmr->tick = now; - if (FIELD_EX8(tmr->tccr[1], TCCR, CSS) == CSS_INTERNAL) { + switch (FIELD_EX8(tmr->tccr[1], TCCR, CSS)) { + case CSS_INTERNAL: /* timer1 count update */ elapsed = elapsed_time(tmr, 1, delta); if (elapsed >= 0x100) { ovf = elapsed >> 8; } tcnt[1] = tmr->tcnt[1] + (elapsed & 0xff); + break; + case CSS_INVALID: /* guest error to have set this */ + case CSS_EXTERNAL: /* QEMU doesn't implement these */ + case CSS_CASCADING: + tcnt[1] = tmr->tcnt[1]; + break; } switch (FIELD_EX8(tmr->tccr[0], TCCR, CSS)) { case CSS_INTERNAL: @@ -144,9 +153,11 @@ static uint16_t read_tcnt(RTMRState *tmr, unsigned size, int ch) tcnt[0] = tmr->tcnt[0] + elapsed; break; case CSS_CASCADING: - if (ovf > 0) { - tcnt[0] = tmr->tcnt[0] + ovf; - } + tcnt[0] = tmr->tcnt[0] + ovf; + break; + case CSS_INVALID: /* guest error to have set this */ + case CSS_EXTERNAL: /* QEMU doesn't implement this */ + tcnt[0] = tmr->tcnt[0]; break; } } else { -- 2.20.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] hw/timer/renesas_tmr: Fix use of uninitialized data in read_tcnt() 2021-02-19 22:32 ` [PATCH 2/2] hw/timer/renesas_tmr: Fix use of uninitialized data in read_tcnt() Peter Maydell @ 2021-02-19 23:13 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 5+ messages in thread From: Philippe Mathieu-Daudé @ 2021-02-19 23:13 UTC (permalink / raw) To: Peter Maydell, qemu-devel; +Cc: Magnus Damm, Yoshinori Sato On 2/19/21 11:32 PM, Peter Maydell wrote: > The read_tcnt() function calculates the TCNT register values for the > two channels of the timer module; it sets these up in the local > tcnt[] array, and eventually returns either one or both of them, > depending on whether the access is 8 or 16 bits. However, not all of > the code paths through this function set both elements of this array: > if the guest has programmed the TCCR.CSS register fields to values > which are either documented as not to be used or which QEMU does not > implement, then the function will return uninitialized data. (This > was spotted by Coverity.) > > Add the missing CSS cases to this code, so that we return a > consistent value instead of uninitialized data, and so the code > structure indicates what's happening. > > Fixes: CID 1429976 > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/timer/renesas_tmr.c | 19 +++++++++++++++---- > 1 file changed, 15 insertions(+), 4 deletions(-) > > diff --git a/hw/timer/renesas_tmr.c b/hw/timer/renesas_tmr.c > index 22260aaaba5..eed39917fec 100644 > --- a/hw/timer/renesas_tmr.c > +++ b/hw/timer/renesas_tmr.c > @@ -46,7 +46,9 @@ REG8(TCCR, 10) > FIELD(TCCR, CSS, 3, 2) > FIELD(TCCR, TMRIS, 7, 1) > > +#define CSS_EXTERNAL 0x00 > #define CSS_INTERNAL 0x01 > +#define CSS_INVALID 0x02 > #define CSS_CASCADING 0x03 > #define CCLR_A 0x01 > #define CCLR_B 0x02 > @@ -130,13 +132,20 @@ static uint16_t read_tcnt(RTMRState *tmr, unsigned size, int ch) > if (delta > 0) { > tmr->tick = now; > > - if (FIELD_EX8(tmr->tccr[1], TCCR, CSS) == CSS_INTERNAL) { > + switch (FIELD_EX8(tmr->tccr[1], TCCR, CSS)) { > + case CSS_INTERNAL: > /* timer1 count update */ > elapsed = elapsed_time(tmr, 1, delta); > if (elapsed >= 0x100) { > ovf = elapsed >> 8; > } > tcnt[1] = tmr->tcnt[1] + (elapsed & 0xff); > + break; > + case CSS_INVALID: /* guest error to have set this */ > + case CSS_EXTERNAL: /* QEMU doesn't implement these */ > + case CSS_CASCADING: > + tcnt[1] = tmr->tcnt[1]; > + break; > } > switch (FIELD_EX8(tmr->tccr[0], TCCR, CSS)) { > case CSS_INTERNAL: > @@ -144,9 +153,11 @@ static uint16_t read_tcnt(RTMRState *tmr, unsigned size, int ch) > tcnt[0] = tmr->tcnt[0] + elapsed; > break; > case CSS_CASCADING: > - if (ovf > 0) { > - tcnt[0] = tmr->tcnt[0] + ovf; > - } > + tcnt[0] = tmr->tcnt[0] + ovf; > + break; > + case CSS_INVALID: /* guest error to have set this */ > + case CSS_EXTERNAL: /* QEMU doesn't implement this */ > + tcnt[0] = tmr->tcnt[0]; > break; > } Elegant nice fix :) Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-02-19 23:14 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-02-19 22:32 [PATCH 0/2] hw/timer/renesas_tmr: Fix use of uninitialized data Peter Maydell 2021-02-19 22:32 ` [PATCH 1/2] hw/timer/renesas_tmr: Prefix constants for CSS values with CSS_ Peter Maydell 2021-02-19 23:10 ` Philippe Mathieu-Daudé 2021-02-19 22:32 ` [PATCH 2/2] hw/timer/renesas_tmr: Fix use of uninitialized data in read_tcnt() Peter Maydell 2021-02-19 23:13 ` Philippe Mathieu-Daudé
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).