On Sun, 30 Jun 2024 at 18:00, Zheyu Ma <zheyum...@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 <zheyum...@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
