From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40667) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YsbxL-0002mh-1Z for qemu-devel@nongnu.org; Wed, 13 May 2015 15:04:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YsbxH-0006FH-Pf for qemu-devel@nongnu.org; Wed, 13 May 2015 15:04:34 -0400 Message-ID: <5553A03D.2030208@redhat.com> Date: Wed, 13 May 2015 15:04:29 -0400 From: John Snow MIME-Version: 1.0 References: <1431527602-29889-1-git-send-email-jsnow@redhat.com> <55539D17.7010000@weilnetz.de> <55539EFA.6050605@profihost.ag> In-Reply-To: <55539EFA.6050605@profihost.ag> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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 Priebe , Stefan Weil , qemu-stable@nongnu.org, peter.maydell@linaro.org Cc: Petr Matousek , qemu-devel@nongnu.org On 05/13/2015 02:59 PM, Stefan Priebe wrote: >=20 > Am 13.05.2015 um 20:51 schrieb Stefan Weil: >> Hi, >> >> I just noticed this patch because my provider told me that my KVM base= d >> server >> needs a reboot because of a CVE (see this German news: >> http://www.heise.de/newsticker/meldung/Venom-Schwachstelle-Aus-Hypervi= sor-ausbrechen-und-VMs-ausspionieren-2649614.html) >> >=20 > Isn't a live migration to a fixed version enough instead of a reboot? >=20 > Stefan >=20 >=20 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 >>> >>> 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 =3D 0; >>> - int pos; >>> + uint32_t pos; >>> cur_drv =3D get_cur_drv(fdctrl); >>> fdctrl->dsr &=3D ~FD_DSR_PWRDOWN; >>> @@ -1506,8 +1506,8 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl= ) >>> return 0; >>> } >>> pos =3D fdctrl->data_pos; >>> + pos %=3D FD_SECTOR_LEN; >> >> I'd combine both statements and perhaps use fdctrl->fifo_size (even if >> the resulting code will be slightly larger): >> >> pos =3D fdctrl->data_pos % fdctrl->fifo_size; >> >> >>> if (fdctrl->msr & FD_MSR_NONDMA) { >>> - pos %=3D FD_SECTOR_LEN; >>> if (pos =3D=3D 0) { >>> if (fdctrl->data_pos !=3D 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 =3D get_cur_drv(fdctrl); >>> + uint32_t pos; >>> - if (fdctrl->fifo[fdctrl->data_pos - 1] & 0x80) { >>> + pos =3D fdctrl->data_pos - 1; >>> + pos %=3D FD_SECTOR_LEN; >> >> Shorter (and more clear): >> >> uint32_t pos =3D (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] =3D fdctrl->fifo[1]; >>> fdctrl->fifo[2] =3D 0; >>> fdctrl->fifo[3] =3D 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++] =3D value; >>> + pos =3D fdctrl->data_pos++; >>> + pos %=3D FD_SECTOR_LEN; >>> + fdctrl->fifo[pos] =3D value; >>> if (fdctrl->data_pos =3D=3D 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 coul= d >> also be improved. >> >> fdctrl->fifo =3D qemu_memalign(512, FD_SECTOR_LEN); >> fdctrl->fifo_size =3D 512; >> >> The 2nd line should be >> >> fdctrl->fifo_size =3D 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 w= as >> claimed) modify the code of the VM host because those memory is usuall= y >> 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 shoul= d >> perhaps consider changing that. >> >> Regards >> Stefan >> >> >=20 --=20 =E2=80=94js