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 <zheyum...@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; }
