Re: [PATCH 2/2] hw/timer/renesas_tmr: Fix use of uninitialized data in read_tcnt()

[Philippe Mathieu-Daudé]

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>