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

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

[Zheyu Ma]

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

Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
---
Changes in v3:
- Defined masks for UARTIBRD and UARTFBRD to avoid magic numbers.
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 | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 8753b84a84..f8078aa216 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -87,6 +87,12 @@ DeviceState *pl011_create(hwaddr addr, qemu_irq irq, Chardev *chr)
 #define CR_DTR      (1 << 10)
 #define CR_LBE      (1 << 7)
 
+/* Integer Baud Rate Divider, UARTIBRD */
+#define IBRD_MASK 0x3f
+
+/* Fractional Baud Rate Divider, UARTFBRD */
+#define FBRD_MASK 0xffff
+
 static const unsigned char pl011_id_arm[8] =
   { 0x11, 0x10, 0x14, 0x00, 0x0d, 0xf0, 0x05, 0xb1 };
 static const unsigned char pl011_id_luminary[8] =
@@ -374,11 +380,11 @@ static void pl011_write(void *opaque, hwaddr offset,
         s->ilpr = value;
         break;
     case 9: /* UARTIBRD */
-        s->ibrd = value;
+        s->ibrd = value & IBRD_MASK;
         pl011_trace_baudrate_change(s);
         break;
     case 10: /* UARTFBRD */
-        s->fbrd = value;
+        s->fbrd = value & FBRD_MASK;
         pl011_trace_baudrate_change(s);
         break;
     case 11: /* UARTLCR_H */
@@ -531,6 +537,9 @@ static int pl011_post_load(void *opaque, int version_id)
         s->read_pos = 0;
     }
 
+    s->ibrd &= IBRD_MASK;
+    s->fbrd &= FBRD_MASK;
+
     return 0;
 }
 
-- 
2.34.1

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

[Philippe Mathieu-Daudé]

On 2/7/24 17:57, 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
> 
> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Peter, feel free to replace that line by:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
> ---
> Changes in v3:
> - Defined masks for UARTIBRD and UARTFBRD to avoid magic numbers.

Thanks Zheyu!

> 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 | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)

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

[Peter Maydell]

On Tue, 2 Jul 2024 at 17:18, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 2/7/24 17:57, 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
> >
> > Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
> Peter, feel free to replace that line by:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

OK; applied to target-arm.next, thanks.

-- PMM