1. Patchew
  2. QEMU
  3. View series

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

Zheyu Ma posted 1 patch 1 year, 7 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20240702152750.3017426-1-zheyuma97@gmail.com
Maintainers: Peter Maydell <peter.maydell@linaro.org>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>
There is a newer version of this series
hw/char/pl011.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
[PATCH v2] hw/char/pl011: Avoid division-by-zero in pl011_get_baudrate()
Posted by Zheyu Ma 1 year, 7 months ago 
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;
         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;
 }
 
-- 
2.34.1
Re: [PATCH v2] hw/char/pl011: Avoid division-by-zero in pl011_get_baudrate()
Posted by Philippe Mathieu-Daudé 1 year, 7 months ago 
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;
>   }
>
Re: [PATCH v2] hw/char/pl011: Avoid division-by-zero in pl011_get_baudrate()
Posted by Philippe Mathieu-Daudé 1 year, 7 months ago 
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;
  }

---
Re: [PATCH v2] hw/char/pl011: Avoid division-by-zero in pl011_get_baudrate()
Posted by Zheyu Ma 1 year, 7 months ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.