Re: [PATCH v2] hw/char/pl011: Avoid division-by-zero in pl011_get_baudrate()

[Philippe Mathieu-Daudé]

Hi Zheyu,

On 2/7/24 17:27, Zheyu Ma wrote:
> In pl011_get_baudrate(), when we calculate the baudrate we can
> accidentally divide by zero. This happens because although (as the
> specification requires) we treat UARTIBRD = 0 as invalid, we aren't
> correctly limiting UARTIBRD and UARTFBRD values to the 16-bit and 6-bit
> ranges the hardware allows, and so some non-zero values of UARTIBRD can
> result in a zero divisor.
>
> Enforce the correct register field widths on guest writes and on inbound
> migration to avoid the division by zero.
>
> ASAN log:
> ==2973125==ERROR: AddressSanitizer: FPE on unknown address 0x55f72629b348
> (pc 0x55f72629b348 bp 0x7fffa24d0e00 sp 0x7fffa24d0d60 T0)
>      #0 0x55f72629b348 in pl011_get_baudrate hw/char/pl011.c:255:17
>      #1 0x55f726298d94 in pl011_trace_baudrate_change hw/char/pl011.c:260:33
>      #2 0x55f726296fc8 in pl011_write hw/char/pl011.c:378:9
>
> Reproducer:
> cat << EOF | qemu-system-aarch64 -display \
> none -machine accel=qtest, -m 512M -machine realview-pb-a8 -qtest stdio
> writeq 0x1000b024 0xf8000000
> EOF
>
> Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
> ---
> Changes in v2:
> - Enforce the correct register field widths on writes to both UARTIBRD
>    and UARTFBRD registers.
> - Mask UARTIBRD to 16 bits and UARTFBRD to 6 bits in the pl011_post_load
>    function to prevent division by zero during inbound migration.
> ---
>   hw/char/pl011.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index 8753b84a84..ba5f7cfbda 100644
> --- a/hw/char/pl011.c
> +++ b/hw/char/pl011.c
> @@ -374,11 +374,11 @@ static void pl011_write(void *opaque, hwaddr offset,
>           s->ilpr = value;
>           break;
>       case 9: /* UARTIBRD */
> -        s->ibrd = value;
> +        s->ibrd = value & 0xffff;

LGTM but could you add a definition instead of these magic values?

Similarly to the ones in the top if this file:

  /* Flag Register, UARTFR */
  #define PL011_FLAG_RI   0x100
  #define PL011_FLAG_TXFE 0x80
  ...

  /* Integer Baud Rate Divider, UARTIBRD */
  #define IBRD_MASK 0x3f

  /* Fractional Baud Rate Divider, UARTFBRD */
  #define FBRD_MASK 0xffff

Then use these definitions :)

Regards,

Phil.

>           pl011_trace_baudrate_change(s);
>           break;
>       case 10: /* UARTFBRD */
> -        s->fbrd = value;
> +        s->fbrd = value & 0x3f;
>           pl011_trace_baudrate_change(s);
>           break;
>       case 11: /* UARTLCR_H */
> @@ -531,6 +531,9 @@ static int pl011_post_load(void *opaque, int version_id)
>           s->read_pos = 0;
>       }
>   
> +    s->ibrd &= 0xffff;
> +    s->fbrd &= 0x3f;
> +
>       return 0;
>   }
>