| Message ID | d30a50ca3497f2a909cd50f07413dd7250078e9d.1478303567.git.alistair.francis@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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 --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;
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(+)