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

[Alistair Francis]

On Sat, Nov 5, 2016 at 6:51 AM, Peter Maydell <address@hidden> wrote:
> On 5 November 2016 at 00:00, Alistair Francis
> <address@hidden> 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 <address@hidden>
>> Signed-off-by: Alistair Francis <address@hidden>
>> ---
>>
>>  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