From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41293) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YsbzD-0004qu-Eb for qemu-devel@nongnu.org; Wed, 13 May 2015 15:06:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ysbz9-00077v-N9 for qemu-devel@nongnu.org; Wed, 13 May 2015 15:06:31 -0400 Received: from mail-ph.de-nserver.de ([85.158.179.214]:49627) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ysbz9-00077K-Cx for qemu-devel@nongnu.org; Wed, 13 May 2015 15:06:27 -0400 Message-ID: <5553A0BC.50708@profihost.ag> Date: Wed, 13 May 2015 21:06:36 +0200 From: Stefan Priebe MIME-Version: 1.0 References: <1431527602-29889-1-git-send-email-jsnow@redhat.com> <55539D17.7010000@weilnetz.de> <55539EFA.6050605@profihost.ag> <5553A03D.2030208@redhat.com> In-Reply-To: <5553A03D.2030208@redhat.com> Content-Type: text/plain; charset=utf-8; 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: John Snow , Stefan Weil , qemu-stable@nongnu.org, peter.maydell@linaro.org Cc: Petr Matousek , qemu-devel@nongnu.org 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 >>>> >>>> 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 >>> >>> >> >