| Message ID | 20240702152750.3017426-1-zheyuma97@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] hw/char/pl011: Avoid division-by-zero in pl011_get_baudrate() | expand |
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 <zheyuma97@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; > } >
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 <zheyuma97@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; } ---
Hi Philippe, On Tue, Jul 2, 2024 at 5:44 PM Philippe Mathieu-Daudé <philmd@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 <zheyuma97@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
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; 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; }
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 <zheyuma97@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(-)