[Qemu-devel] [PATCH for 2.8 v3 1/1] cadence_uart: Check baud rate generator and divider values on migration

[Qemu-devel] [PATCH for 2.8 v3 1/1] cadence_uart: Check baud rate generator and divider values on migration

[Alistair Francis]

The Cadence UART device emulator calculates speed by dividing the
baud rate by a 'baud rate generator' & 'baud rate divider' value.
The device specification defines these register values to be
non-zero and within certain limits. Checks were recently added when
writing to these registers but not when restoring from migration.

This patch adds checks when restoring from migration to avoid divide by
zero errors.

Reported-by: Huawei PSIRT <psirt@huawei.com>
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
It would be nice to squeeze this into 2.8 if possible.

V3:
 - Fix broken migration logic
 - Manually double checked and it passes migration.
V2:
 - Abort the migration if the data is invalid

 hw/char/cadence_uart.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index 0215d65..ce9063b 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -502,6 +502,13 @@ static int cadence_uart_post_load(void *opaque, int version_id)
 {
     CadenceUARTState *s = opaque;
 
+    /* Ensure these two aren't invalid numbers */
+    if (s->r[R_BRGR] <= 1 || s->r[R_BRGR] & ~0xFFFF ||
+        s->r[R_BDIV] <= 3 || s->r[R_BDIV] & ~0xFF) {
+        /* Value is invalid, abort */
+        return 1;
+    }
+
     uart_parameters_setup(s);
     uart_update_status(s);
     return 0;
-- 
2.7.4

Re: [Qemu-devel] [PATCH for 2.8 v3 1/1] cadence_uart: Check baud rate generator and divider values on migration

[Huawei PSIRT]

Dear Alistair francis,

As the issue is found by Jiangxin from Huawei test team, could you please
change the "Reported-by" to Jiangxin <jiangxin1@huawei.com>, thank you.
And keep Jiangxin <jiangxin1@huawei.com> in the email loop, thank you.

Best regards,
Huawei PSIRT

-----邮件原件-----
发件人: Alistair Francis [mailto:alistair.francis@xilinx.com] 
发送时间: 2016年12月6日 2:35
收件人: qemu-devel@nongnu.org; peter.maydell@linaro.org
抄送: alistair.francis@xilinx.com; alistair23@gmail.com;
pjp@fedoraproject.org; psirt@huawei.com
主题: [PATCH for 2.8 v3 1/1] cadence_uart: Check baud rate generator and
divider values on migration

The Cadence UART device emulator calculates speed by dividing the baud rate
by a 'baud rate generator' & 'baud rate divider' value.
The device specification defines these register values to be non-zero and
within certain limits. Checks were recently added when writing to these
registers but not when restoring from migration.

This patch adds checks when restoring from migration to avoid divide by zero
errors.

Reported-by: Huawei PSIRT <psirt@huawei.com>
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
It would be nice to squeeze this into 2.8 if possible.

V3:
 - Fix broken migration logic
 - Manually double checked and it passes migration.
V2:
 - Abort the migration if the data is invalid

 hw/char/cadence_uart.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c index 0215d65..
ce9063b 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -502,6 +502,13 @@ static int cadence_uart_post_load(void *opaque, int
version_id)  {
     CadenceUARTState *s = opaque;
 
+    /* Ensure these two aren't invalid numbers */
+    if (s->r[R_BRGR] <= 1 || s->r[R_BRGR] & ~0xFFFF ||
+        s->r[R_BDIV] <= 3 || s->r[R_BDIV] & ~0xFF) {
+        /* Value is invalid, abort */
+        return 1;
+    }
+
     uart_parameters_setup(s);
     uart_update_status(s);
     return 0;
--
2.7.4

Re: [Qemu-devel] [PATCH for 2.8 v3 1/1] cadence_uart: Check baud rate generator and divider values on migration

[Peter Maydell]

On 5 December 2016 at 18:35, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> The Cadence UART device emulator calculates speed by dividing the
> baud rate by a 'baud rate generator' & 'baud rate divider' value.
> The device specification defines these register values to be
> non-zero and within certain limits. Checks were recently added when
> writing to these registers but not when restoring from migration.
>
> This patch adds checks when restoring from migration to avoid divide by
> zero errors.
>
> Reported-by: Huawei PSIRT <psirt@huawei.com>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
> It would be nice to squeeze this into 2.8 if possible.
>
> V3:
>  - Fix broken migration logic
>  - Manually double checked and it passes migration.
> V2:
>  - Abort the migration if the data is invalid
>
>  hw/char/cadence_uart.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
> index 0215d65..ce9063b 100644
> --- a/hw/char/cadence_uart.c
> +++ b/hw/char/cadence_uart.c
> @@ -502,6 +502,13 @@ static int cadence_uart_post_load(void *opaque, int version_id)
>  {
>      CadenceUARTState *s = opaque;
>
> +    /* Ensure these two aren't invalid numbers */
> +    if (s->r[R_BRGR] <= 1 || s->r[R_BRGR] & ~0xFFFF ||
> +        s->r[R_BDIV] <= 3 || s->r[R_BDIV] & ~0xFF) {

The uart_write() code says BRGR == 1 is valid, but
this code says it isn't. Which is correct?

thanks
-- PMM

Re: [Qemu-devel] [PATCH for 2.8 v3 1/1] cadence_uart: Check baud rate generator and divider values on migration

[Alistair Francis]

On Tue, Dec 6, 2016 at 1:31 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 5 December 2016 at 18:35, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> The Cadence UART device emulator calculates speed by dividing the
>> baud rate by a 'baud rate generator' & 'baud rate divider' value.
>> The device specification defines these register values to be
>> non-zero and within certain limits. Checks were recently added when
>> writing to these registers but not when restoring from migration.
>>
>> This patch adds checks when restoring from migration to avoid divide by
>> zero errors.
>>
>> Reported-by: Huawei PSIRT <psirt@huawei.com>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>> It would be nice to squeeze this into 2.8 if possible.
>>
>> V3:
>>  - Fix broken migration logic
>>  - Manually double checked and it passes migration.
>> V2:
>>  - Abort the migration if the data is invalid
>>
>>  hw/char/cadence_uart.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
>> index 0215d65..ce9063b 100644
>> --- a/hw/char/cadence_uart.c
>> +++ b/hw/char/cadence_uart.c
>> @@ -502,6 +502,13 @@ static int cadence_uart_post_load(void *opaque, int version_id)
>>  {
>>      CadenceUARTState *s = opaque;
>>
>> +    /* Ensure these two aren't invalid numbers */
>> +    if (s->r[R_BRGR] <= 1 || s->r[R_BRGR] & ~0xFFFF ||
>> +        s->r[R_BDIV] <= 3 || s->r[R_BDIV] & ~0xFF) {
>
> The uart_write() code says BRGR == 1 is valid, but
> this code says it isn't. Which is correct?

1 is valid, it is bypass mode, fixing.

Thanks,

Alistair

>
> thanks
> -- PMM