fdc: force the fifo access to be in bounds of the allocated buffer

From: Petr Matousek <[email protected]>

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 <[email protected]> Reviewed-by:

John Snow <[email protected]> Signed-off-by: John Snow <[email protected]> ---

From: Petr Matousek <[email protected]>

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 <[email protected]>

Reviewed-by John Snow <[email protected]>

Signed-off-by John Snow <[email protected]>

---
  hw/block/fdc.c | 17 +++++++++++------
  1 file changed, 11 insertions(+), 6 deletions(-)

hw/block/fdc.c +1 -1

index f72a392..d8a8edd 100644
@@ -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;

hw/block/fdc.c +0 -1

      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)) {

hw/block/fdc.c +3 -2

+    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)) {

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:

Quoted 32 lines

From: Petr Matousek <[email protected]>

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 <[email protected]>

Reviewed-by John Snow <[email protected]>

Signed-off-by John Snow <[email protected]>

---
  hw/block/fdc.c | 17 +++++++++++------
  1 file changed, 11 insertions(+), 6 deletions(-)

hw/block/fdc.c +1 -1

index f72a392..d8a8edd 100644
@@ -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;

hw/block/fdc.c static uint32_t fdctrl_read_data(FDCtrl *fdctrl) · line 1509

Quoted 4 lines

hw/block/fdc.c +1 -0

@@ -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;

Quoted 6 lines

hw/block/fdc.c +0 -1

      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)) {

hw/block/fdc.c static void fdctrl_handle_option(FDCtrl · line 1859

Quoted 9 lines

hw/block/fdc.c +3 -1

@@ -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;

Quoted 16 lines

hw/block/fdc.c +2 -1

+    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;

hw/block/fdc.c +1 -1

@@ -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)) {

hw/block/fdc.c static void fdctrl_write_data(FDCtrl *fdctrl, · line 2015

Quoted 10 lines

hw/block/fdc.c +3 -1

@@ -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

Am 13.05.2015 um 20:51 schrieb Stefan Weil:

Quoted 7 lines

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

Quoted 114 lines

Am 13.05.2015 um 16:33 schrieb John Snow:

Quoted 32 lines

From: Petr Matousek <[email protected]>

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 <[email protected]>

Reviewed-by John Snow <[email protected]>

Signed-off-by John Snow <[email protected]>

---
  hw/block/fdc.c | 17 +++++++++++------
  1 file changed, 11 insertions(+), 6 deletions(-)

hw/block/fdc.c +1 -1

index f72a392..d8a8edd 100644
@@ -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;

hw/block/fdc.c static uint32_t fdctrl_read_data(FDCtrl *fdctrl) · line 1509

Quoted 4 lines

hw/block/fdc.c +1 -0

@@ -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;

Quoted 6 lines

hw/block/fdc.c +0 -1

      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)) {

hw/block/fdc.c static void fdctrl_handle_option(FDCtrl · line 1859

Quoted 9 lines

hw/block/fdc.c +3 -1

@@ -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;

Quoted 16 lines

hw/block/fdc.c +2 -1

+    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;

hw/block/fdc.c +1 -1

@@ -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)) {

hw/block/fdc.c static void fdctrl_write_data(FDCtrl *fdctrl, · line 2015

Quoted 10 lines

hw/block/fdc.c +3 -1

@@ -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

Quoted 10 lines

Am 13.05.2015 um 20:51 schrieb Stefan Weil:

Quoted 7 lines

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)

Quoted 5 lines

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.

Quoted 116 lines

Quoted 115 lines

Am 13.05.2015 um 16:33 schrieb John Snow:

Quoted 32 lines

From: Petr Matousek <[email protected]>

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 <[email protected]>

Reviewed-by John Snow <[email protected]>

Signed-off-by John Snow <[email protected]>

---
   hw/block/fdc.c | 17 +++++++++++------
   1 file changed, 11 insertions(+), 6 deletions(-)

hw/block/fdc.c +1 -1

index f72a392..d8a8edd 100644
@@ -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;

hw/block/fdc.c static uint32_t fdctrl_read_data(FDCtrl *fdctrl) · line 1509

Quoted 4 lines

hw/block/fdc.c +1 -0

@@ -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;

Quoted 6 lines

hw/block/fdc.c +0 -1

       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)) {

hw/block/fdc.c static void fdctrl_handle_option(FDCtrl · line 1859

Quoted 9 lines

hw/block/fdc.c +3 -1

@@ -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;

Quoted 16 lines

hw/block/fdc.c +2 -1

+    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;

hw/block/fdc.c +1 -1

@@ -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)) {

hw/block/fdc.c static void fdctrl_write_data(FDCtrl *fdctrl, · line 2015

Quoted 10 lines

hw/block/fdc.c +3 -1

@@ -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

Am 13.05.2015 um 20:51 schrieb Stefan Weil:

Quoted 7 lines

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 20:59 schrieb Stefan Priebe:

Quoted 10 lines

Am 13.05.2015 um 20:51 schrieb Stefan Weil:

Quoted 7 lines

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)

Quoted 3 lines

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

Stefan

Good point. A live migration would be sufficient - if there are no bugs in QEMU's live migration.

Stefan

Am 13.05.2015 um 21:05 schrieb Stefan Weil:

Quoted 11 lines

Am 13.05.2015 um 20:59 schrieb Stefan Priebe:

Quoted 10 lines

Am 13.05.2015 um 20:51 schrieb Stefan Weil:

Quoted 7 lines

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)

Quoted 6 lines

Quoted 3 lines

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

Stefan

Good point. A live migration would be sufficient - if there are no bugs in QEMU's live migration.

just migrating all our customer machines and wanted to be sure that live migration is enough.

Am 13.05.2015 um 21:09 schrieb Stefan Priebe:

Quoted 16 lines

Am 13.05.2015 um 21:05 schrieb Stefan Weil:

Quoted 15 lines

Am 13.05.2015 um 20:59 schrieb Stefan Priebe:

Quoted 13 lines

Am 13.05.2015 um 20:51 schrieb Stefan Weil:

Quoted 7 lines

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

Quoted 5 lines

Quoted 2 lines

Good point. A live migration would be sufficient - if there are no bugs in QEMU's live migration.

just migrating all our customer machines and wanted to be sure that live migration is enough.

Just to confirm: If Qemu is started with -nodefaults and there is no fdc configuration the system is not affected by this CVE?

Quoted 14 lines · Peter Lieven

Am 13.05.2015 um 21:09 schrieb Stefan Priebe:

Quoted 13 lines

Am 13.05.2015 um 21:05 schrieb Stefan Weil:

Quoted 12 lines

Am 13.05.2015 um 20:59 schrieb Stefan Priebe:

Quoted 11 lines

Am 13.05.2015 um 20:51 schrieb Stefan Weil:

Quoted 7 lines

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

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

Good point. A live migration would be sufficient - if there are no bugs in QEMU's live migration.

just migrating all our customer machines and wanted to be sure that live migration is enough.

Just to confirm: If Qemu is started with -nodefaults and there is no fdc configuration the system is not affected by this CVE?

Not true. The FD controller is still there. It has no drives attached then, but is vulnerable all the same.

Am 13.05.2015 um 21:52 schrieb Markus Armbruster:

Quoted 16 lines

Quoted 14 lines · Peter Lieven

Am 13.05.2015 um 21:09 schrieb Stefan Priebe:

Quoted 13 lines

Am 13.05.2015 um 21:05 schrieb Stefan Weil:

Quoted 12 lines

Am 13.05.2015 um 20:59 schrieb Stefan Priebe:

Quoted 11 lines

Am 13.05.2015 um 20:51 schrieb Stefan Weil:

Quoted 7 lines

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

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

Good point. A live migration would be sufficient - if there are no bugs in QEMU's live migration.

just migrating all our customer machines and wanted to be sure that live migration is enough.

Just to confirm: If Qemu is started with -nodefaults and there is no fdc configuration the system is not affected by this CVE?

Not true. The FD controller is still there. It has no drives attached then, but is vulnerable all the same.

Are you sure? With -nodefaults the hmp command 'info block' returns nothing and the guest sees no floppy drive.

Without -nodefaults I indeed see floppy0 and I have /dev/fd0 in the guest respectively.

Peter

Quoted 25 lines

Am 13.05.2015 um 21:52 schrieb Markus Armbruster:

Quoted 24 lines

Quoted 13 lines · Peter Lieven

Am 13.05.2015 um 21:09 schrieb Stefan Priebe:

Quoted 12 lines

Am 13.05.2015 um 21:05 schrieb Stefan Weil:

Quoted 11 lines

Am 13.05.2015 um 20:59 schrieb Stefan Priebe:

Quoted 10 lines

Am 13.05.2015 um 20:51 schrieb Stefan Weil:

Quoted 7 lines

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?

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Stefan

Good point. A live migration would be sufficient - if there are no bugs in QEMU's live migration.

just migrating all our customer machines and wanted to be sure that live migration is enough.

Just to confirm: If Qemu is started with -nodefaults and there is no fdc configuration the system is not affected by this CVE?

Not true. The FD controller is still there. It has no drives attached then, but is vulnerable all the same.

Quoted 7 lines

Are you sure? With -nodefaults the hmp command 'info block' returns nothing and the guest sees no floppy drive.

Without -nodefaults I indeed see floppy0 and I have /dev/fd0 in the guest respectively.

Peter

It's not the *drive* that is the problem, it is the *controller*.

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:

Quoted 31 lines

From: Petr Matousek <[email protected]>

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 <[email protected]>

Reviewed-by John Snow <[email protected]>

Signed-off-by John Snow <[email protected]>

---
  hw/block/fdc.c | 17 +++++++++++------
  1 file changed, 11 insertions(+), 6 deletions(-)

hw/block/fdc.c +1 -1

index f72a392..d8a8edd 100644
@@ -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;

hw/block/fdc.c static uint32_t fdctrl_read_data(FDCtrl *fdctrl) · line 1509

Quoted 5 lines

hw/block/fdc.c +1 -0

@@ -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;

Quoted 6 lines

hw/block/fdc.c +0 -1

      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)) {

hw/block/fdc.c static void fdctrl_handle_option(FDCtrl · line 1860

Quoted 9 lines

hw/block/fdc.c +3 -0

@@ -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;

Quoted 16 lines

hw/block/fdc.c +3 -2

+    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)) {

hw/block/fdc.c static void fdctrl_write_data(FDCtrl *fdctrl, · line 2015

Quoted 10 lines

hw/block/fdc.c +3 -1

@@ -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