Hi Philippe,
On Tue, Jul 2, 2024 at 5:44 PM Philippe Mathieu-Daudé <phi...@linaro.org>
wrote:
> On 2/7/24 17:40, Philippe Mathieu-Daudé wrote:
> > 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 :)
>
> Since I have the patch, let's share it to save you some time:
>
> -- >8 --
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index 8753b84a84..7f9a85708e 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;
> }
>
Thanks for your helpful patch :)
regards,
Zheyu
