Re: [Qemu-stable] [Qemu-devel] [PATCH] fdc: force the fifo access to be in bounds of the allocated buffer

[Stefan Priebe]

Am 13.05.2015 um 21:04 schrieb John Snow:
> On 05/13/2015 02:59 PM, Stefan Priebe wrote:
> > Am 13.05.2015 um 20:51 schrieb Stefan Weil:
> > > Hi,
> > >
> > > I just noticed this patch because my provider told me that my KVM based
> > > server
> > > needs a reboot because of a CVE (see this German news:
> > > http://www.heise.de/newsticker/meldung/Venom-Schwachstelle-Aus-Hypervisor-ausbrechen-und-VMs-ausspionieren-2649614.html)
> >
> > Isn't a live migration to a fixed version enough instead of a reboot?
> >
> > Stefan
>
> If your management API or host or whatever lets you migrate back to the
> same host, or has another host they can migrate you to, yes.

i'm my host ;-) I was just interested if for whatever reason live 
migration is not enough and was wondering why someone wants to reboot a 
vm for something like this.

Thanks!

Stefan

> > > Am 13.05.2015 um 16:33 schrieb John Snow:
> > > > From: Petr Matousek <address@hidden>
> > > >
> > > > During processing of certain commands such as FD_CMD_READ_ID and
> > > > FD_CMD_DRIVE_SPECIFICATION_COMMAND the fifo memory access could
> > > > get out of bounds leading to memory corruption with values coming
> > > > from the guest.
> > > >
> > > > Fix this by making sure that the index is always bounded by the
> > > > allocated memory.
> > > >
> > > > This is CVE-2015-3456.
> > > >
> > > > Signed-off-by: Petr Matousek <address@hidden>
> > > > Reviewed-by: John Snow <address@hidden>
> > > > Signed-off-by: John Snow <address@hidden>
> > > > ---
> > > >    hw/block/fdc.c | 17 +++++++++++------
> > > >    1 file changed, 11 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> > > > index f72a392..d8a8edd 100644
> > > > --- a/hw/block/fdc.c
> > > > +++ b/hw/block/fdc.c
> > > > @@ -1497,7 +1497,7 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
> > > >    {
> > > >        FDrive *cur_drv;
> > > >        uint32_t retval = 0;
> > > > -    int pos;
> > > > +    uint32_t pos;
> > > >        cur_drv = get_cur_drv(fdctrl);
> > > >        fdctrl->dsr &= ~FD_DSR_PWRDOWN;
> > > > @@ -1506,8 +1506,8 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
> > > >            return 0;
> > > >        }
> > > >        pos = fdctrl->data_pos;
> > > > +    pos %= FD_SECTOR_LEN;
> > >
> > > I'd combine both statements and perhaps use fdctrl->fifo_size (even if
> > > the resulting code will be slightly larger):
> > >
> > > pos = fdctrl->data_pos % fdctrl->fifo_size;
> > >
> > >
> > > >        if (fdctrl->msr & FD_MSR_NONDMA) {
> > > > -        pos %= FD_SECTOR_LEN;
> > > >            if (pos == 0) {
> > > >                if (fdctrl->data_pos != 0)
> > > >                    if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) {
> > > > @@ -1852,10 +1852,13 @@ static void fdctrl_handle_option(FDCtrl
> > > > *fdctrl, int direction)
> > > >    static void fdctrl_handle_drive_specification_command(FDCtrl
> > > > *fdctrl, int direction)
> > > >    {
> > > >        FDrive *cur_drv = get_cur_drv(fdctrl);
> > > > +    uint32_t pos;
> > > > -    if (fdctrl->fifo[fdctrl->data_pos - 1] & 0x80) {
> > > > +    pos = fdctrl->data_pos - 1;
> > > > +    pos %= FD_SECTOR_LEN;
> > >
> > > Shorter (and more clear):
> > >
> > > uint32_t pos = (fdctrl->data_pos - 1) % fdctrl->fifo_size;
> > >
> > > > +    if (fdctrl->fifo[pos] & 0x80) {
> > > >            /* Command parameters done */
> > > > -        if (fdctrl->fifo[fdctrl->data_pos - 1] & 0x40) {
> > > > +        if (fdctrl->fifo[pos] & 0x40) {
> > > >                fdctrl->fifo[0] = fdctrl->fifo[1];
> > > >                fdctrl->fifo[2] = 0;
> > > >                fdctrl->fifo[3] = 0;
> > > > @@ -1955,7 +1958,7 @@ static uint8_t command_to_handler[256];
> > > >    static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
> > > >    {
> > > >        FDrive *cur_drv;
> > > > -    int pos;
> > > > +    uint32_t pos;
> > > >        /* Reset mode */
> > > >        if (!(fdctrl->dor & FD_DOR_nRESET)) {
> > > > @@ -2004,7 +2007,9 @@ static void fdctrl_write_data(FDCtrl *fdctrl,
> > > > uint32_t value)
> > > >        }
> > > >        FLOPPY_DPRINTF("%s: %02x\n", __func__, value);
> > > > -    fdctrl->fifo[fdctrl->data_pos++] = value;
> > > > +    pos = fdctrl->data_pos++;
> > > > +    pos %= FD_SECTOR_LEN;
> > > > +    fdctrl->fifo[pos] = value;
> > > >        if (fdctrl->data_pos == fdctrl->data_len) {
> > > >            /* We now have all parameters
> > > >             * and will be able to treat the command
> > >
> > > Not strictly related to this patch: The code which sets fifo_size could
> > > also be improved.
> > >
> > >       fdctrl->fifo = qemu_memalign(512, FD_SECTOR_LEN);
> > >       fdctrl->fifo_size = 512;
> > >
> > > The 2nd line should be
> > >
> > >       fdctrl->fifo_size = FD_SECTOR_LEN;
> > >
> > >
> > > As far as I see the original code can read or write illegal memory
> > > locations in the address space of the QEMU process. It cannot (as it was
> > > claimed) modify the code of the VM host because those memory is usually
> > > write protected - at least if QEMU is running without KVM. If the code
> > > which is generated for KVM is writable from anywhere in QEMU, we should
> > > perhaps consider changing that.
> > >
> > > Regards
> > > Stefan