diff mbox

[v1,1/1] cadence_uart: Check baud rate generator and divider values on migration

Message ID d30a50ca3497f2a909cd50f07413dd7250078e9d.1478303567.git.alistair.francis@xilinx.com
State New
Headers show

Commit Message

Alistair Francis Nov. 5, 2016, midnight UTC 
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>
---

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

Comments

Peter Maydell Nov. 5, 2016, 1:51 p.m. UTC | #1 
On 5 November 2016 at 00:00, 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>
> ---
>
>  hw/char/cadence_uart.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
> index def34cd..e3a6248 100644
> --- a/hw/char/cadence_uart.c
> +++ b/hw/char/cadence_uart.c
> @@ -487,6 +487,19 @@ 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) {
> +        /* Value is invalid, reset it */
> +        s->r[R_BRGR] = 0x0000028B;
> +    }
> +    if (s->r[R_BDIV] <= 3) {
> +        /* Value is invalid, reset it */
> +        s->r[R_BDIV] = 0x0000000F;
> +    }
> +
> +    s->r[R_BRGR] = s->r[R_BRGR] & 0xFFFF;
> +    s->r[R_BDIV] = s->r[R_BDIV] & 0xFF;
> +
>      uart_parameters_setup(s);
>      uart_update_status(s);
>      return 0;


Usually we just fail the migration if the incoming
data is bogus -- any particular reason not to take that
approach here?

thanks
-- PMM
Alistair Francis Nov. 7, 2016, 9:53 p.m. UTC | #2 
On Sat, Nov 5, 2016 at 6:51 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 5 November 2016 at 00:00, 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>
>> ---
>>
>>  hw/char/cadence_uart.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
>> index def34cd..e3a6248 100644
>> --- a/hw/char/cadence_uart.c
>> +++ b/hw/char/cadence_uart.c
>> @@ -487,6 +487,19 @@ 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) {
>> +        /* Value is invalid, reset it */
>> +        s->r[R_BRGR] = 0x0000028B;
>> +    }
>> +    if (s->r[R_BDIV] <= 3) {
>> +        /* Value is invalid, reset it */
>> +        s->r[R_BDIV] = 0x0000000F;
>> +    }
>> +
>> +    s->r[R_BRGR] = s->r[R_BRGR] & 0xFFFF;
>> +    s->r[R_BDIV] = s->r[R_BDIV] & 0xFF;
>> +
>>      uart_parameters_setup(s);
>>      uart_update_status(s);
>>      return 0;
>
>
> Usually we just fail the migration if the incoming
> data is bogus -- any particular reason not to take that
> approach here?

There is no reason, it just seemed a bit much to abort just for this.

Should I change it to abort?

Thanks,

Alistair

>
> thanks
> -- PMM
Peter Maydell Nov. 7, 2016, 10:13 p.m. UTC | #3 
On 7 November 2016 at 21:53, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> On Sat, Nov 5, 2016 at 6:51 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Usually we just fail the migration if the incoming
>> data is bogus -- any particular reason not to take that
>> approach here?
>
> There is no reason, it just seemed a bit much to abort just for this.
>
> Should I change it to abort?

I think there are two cases:
 (1) migration from an old version could be in these
bogus states (without having crashed the old version
in the process) -- in that case you can argue for
sanitizing as being most helpful to the user
(and should comment that that's why we accept-but-squash)
 (2) the out-of-bounds values only happen if somebody
is deliberately feeding QEMU a bogus incoming data
stream -- in this case (which is the usual one for
bounds checks) it's best to return 1 to fail the
migration.

thanks
-- PMM
Alistair Francis Nov. 7, 2016, 10:43 p.m. UTC | #4 
On Mon, Nov 7, 2016 at 2:13 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 7 November 2016 at 21:53, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> On Sat, Nov 5, 2016 at 6:51 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> Usually we just fail the migration if the incoming
>>> data is bogus -- any particular reason not to take that
>>> approach here?
>>
>> There is no reason, it just seemed a bit much to abort just for this.
>>
>> Should I change it to abort?
>
> I think there are two cases:
>  (1) migration from an old version could be in these
> bogus states (without having crashed the old version
> in the process) -- in that case you can argue for
> sanitizing as being most helpful to the user
> (and should comment that that's why we accept-but-squash)

I think this is actually very unlikely, anyone setting these values by
accident has probably already seen crashes.

>  (2) the out-of-bounds values only happen if somebody
> is deliberately feeding QEMU a bogus incoming data
> stream -- in this case (which is the usual one for
> bounds checks) it's best to return 1 to fail the
> migration.

This seems more likely, so it sounds like I should fail the migration.

Thanks,

Alistair

>
> thanks
> -- PMM
>
diff mbox

Patch

diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index def34cd..e3a6248 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -487,6 +487,19 @@  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) {
+        /* Value is invalid, reset it */
+        s->r[R_BRGR] = 0x0000028B;
+    }
+    if (s->r[R_BDIV] <= 3) {
+        /* Value is invalid, reset it */
+        s->r[R_BDIV] = 0x0000000F;
+    }
+
+    s->r[R_BRGR] = s->r[R_BRGR] & 0xFFFF;
+    s->r[R_BDIV] = s->r[R_BDIV] & 0xFF;
+
     uart_parameters_setup(s);
     uart_update_status(s);
     return 0;