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. >> >> Am 13.05.2015 um 16:33 schrieb John Snow: >>> From: Petr Matousek <pmato...@redhat.com> >>> >>> 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 <pmato...@redhat.com> >>> Reviewed-by: John Snow <js...@redhat.com> >>> Signed-off-by: John Snow <js...@redhat.com> >>> --- >>> 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 >> >> > -- —js