Re: [PATCH] hw/char/pl011: ensure UARTIBRD register is 16-bit

[Peter Maydell]

On Sun, 30 Jun 2024 at 18:00, Zheyu Ma <zheyuma97@gmail.com> wrote:
>
> The PL011 TRM says that "The 16-bit integer is written to the Integer Baud 
> Rate
> Register, UARTIBRD". Updated the handling of the UARTIBRD register to ensure
> only 16-bit values are written to it.

Thanks for this patch.

I think we could improve the patch commit message:
Have the subject state the problem we're fixing rather than
the solution:
 hw/char/pl011: Avoid division-by-zero in pl011_get_baudrate()

and then have the commit message say why this happens
and why we make the change we do:

 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 5-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.

(I mention here a few things which I'm about to comment on below.)

> 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>
> ---
>  hw/char/pl011.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index 8753b84a84..f962786e2a 100644
> --- a/hw/char/pl011.c
> +++ b/hw/char/pl011.c
> @@ -374,7 +374,7 @@ static void pl011_write(void *opaque, hwaddr offset,
>          s->ilpr = value;
>          break;
>      case 9: /* UARTIBRD */
> -        s->ibrd = value;
> +        s->ibrd = value & 0xFFFF;

This is necessary but not sufficient:
 * we also need to mask the write to s->fbrd (which is 6 bits);
   otherwise you can arrange a combination of s->ibrd
   and s->fbrd such that the addition in the divisor
   exactly overflows to 0
 * we should mask these also in pl011_post_load(), to prevent
   the division-by-zero in the "malicious inbound migration
   state" case.

Also, this source file (like most in QEMU) uses lower case
in hex numbers, i.e. 0xffff .

>          pl011_trace_baudrate_change(s);
>          break;
>      case 10: /* UARTFBRD */
> --
> 2.34.1

thanks
-- PMM