From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38899) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ysbry-0000in-Ve for qemu-devel@nongnu.org; Wed, 13 May 2015 14:59:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ysbrv-0003fp-JS for qemu-devel@nongnu.org; Wed, 13 May 2015 14:59:02 -0400 Received: from mail-ph.de-nserver.de ([85.158.179.214]:62234) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ysbrv-0003eK-9K for qemu-devel@nongnu.org; Wed, 13 May 2015 14:58:59 -0400 Message-ID: <55539EFA.6050605@profihost.ag> Date: Wed, 13 May 2015 20:59:06 +0200 From: Stefan Priebe MIME-Version: 1.0 References: <1431527602-29889-1-git-send-email-jsnow@redhat.com> <55539D17.7010000@weilnetz.de> In-Reply-To: <55539D17.7010000@weilnetz.de> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [Qemu-stable] [PATCH] fdc: force the fifo access to be in bounds of the allocated buffer List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Weil , John Snow , qemu-stable@nongnu.org, peter.maydell@linaro.org Cc: Petr Matousek , qemu-devel@nongnu.org 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 > > Am 13.05.2015 um 16:33 schrieb John Snow: >> From: Petr Matousek >> >> 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 >> Reviewed-by: John Snow >> Signed-off-by: John Snow >> --- >> 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 > >