| Message ID | da4de399041146a5d0ede188c5881eaaf93ff930.1478565183.git.alistair.francis@xilinx.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Nov 7, 2016 at 4:34 PM, 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. Ping! Thanks, Alistair > > Reported-by: Huawei PSIRT <psirt@huawei.com> > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> > --- > 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 def34cd..9568ac6 100644 > --- a/hw/char/cadence_uart.c > +++ b/hw/char/cadence_uart.c > @@ -487,6 +487,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 >
On 8 November 2016 at 00:34, 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> > --- > 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 def34cd..9568ac6 100644 > --- a/hw/char/cadence_uart.c > +++ b/hw/char/cadence_uart.c > @@ -487,6 +487,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; > + } The "s->r[R_BRGR] & 0xFFFF" and "s->r[R_BDIV] & 0xFF" checks look wrong -- it's ok for the low bits to be set, it's only if high bits are set that we want to abort the migration. Missing '~'s ? (I'm surprised this bug doesn't cause migration to fail every time.) thanks -- PMM
On Mon, Nov 28, 2016 at 3:31 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 8 November 2016 at 00:34, 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> >> --- >> 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 def34cd..9568ac6 100644 >> --- a/hw/char/cadence_uart.c >> +++ b/hw/char/cadence_uart.c >> @@ -487,6 +487,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; >> + } > > The "s->r[R_BRGR] & 0xFFFF" and "s->r[R_BDIV] & 0xFF" checks > look wrong -- it's ok for the low bits to be set, it's only > if high bits are set that we want to abort the migration. > Missing '~'s ? (I'm surprised this bug doesn't cause migration > to fail every time.) Yep, you are right. Those are wrong. I'll fix them up. I didn't have a migration test case to work with. I will set something up this time to make sure something like that doesn't slip though. Thanks, Alistair > > thanks > -- PMM
diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c index def34cd..9568ac6 100644 --- a/hw/char/cadence_uart.c +++ b/hw/char/cadence_uart.c @@ -487,6 +487,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;
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> --- V2: - Abort the migration if the data is invalid hw/char/cadence_uart.c | 7 +++++++ 1 file changed, 7 insertions(+)