[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [Qemu-stable] [PATCH] fdc: force the fifo access to be

From: Stefan Priebe
Subject: Re: [Qemu-devel] [Qemu-stable] [PATCH] fdc: force the fifo access to be in bounds of the allocated buffer
Date: Wed, 13 May 2015 21:06:36 +0200
User-agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0

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:

I just noticed this patch because my provider told me that my KVM based
needs a reboot because of a CVE (see this German news:

Isn't a live migration to a fixed version enough instead of a reboot?


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.



Am 13.05.2015 um 16:33 schrieb John Snow:
From: Petr Matousek <address@hidden>

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 <address@hidden>
Reviewed-by: John Snow <address@hidden>
Signed-off-by: John Snow <address@hidden>
   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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]