On Sun, 30 Jun 2024 at 18:00, Zheyu Ma 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 0xf800
> EOF
>
> Signed-off-by: Zheyu Ma
> ---
> 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 & 0x;
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. 0x .
> pl011_trace_baudrate_change(s);
> break;
> case 10: /* UARTFBRD */
> --
> 2.34.1
thanks
-- PMM
