From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35705) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YsbkP-00065s-5z for qemu-devel@nongnu.org; Wed, 13 May 2015 14:51:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YsbkL-0000EX-3R for qemu-devel@nongnu.org; Wed, 13 May 2015 14:51:13 -0400 Message-ID: <55539D17.7010000@weilnetz.de> Date: Wed, 13 May 2015 20:51:03 +0200 From: Stefan Weil MIME-Version: 1.0 References: <1431527602-29889-1-git-send-email-jsnow@redhat.com> In-Reply-To: <1431527602-29889-1-git-send-email-jsnow@redhat.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [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: John Snow , qemu-stable@nongnu.org, peter.maydell@linaro.org Cc: Petr Matousek , qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com 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) 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