RfC: add support for deprecated devices.

---
 hw/core/qdev.c         | 9 ++++++++-
 include/hw/qdev-core.h | 1 +
 qdev-monitor.c         | 7 +++++++
 3 files changed, 16 insertions(+), 1 deletion(-)

hw/core/qdev.c +8 -1

index 046d8f1..3b27a74 100644
@@ -133,11 +133,18 @@ DeviceState *qdev_create(BusState *bus, const char *name)
 
 DeviceState *qdev_try_create(BusState *bus, const char *type)
 {
+    DeviceClass *dc;
     DeviceState *dev;
 
-    if (object_class_by_name(type) == NULL) {
+    dc = DEVICE_CLASS(object_class_by_name(type));
+    if (dc == NULL) {
         return NULL;
     }
+    if (dc->deprecation_reason) {
+        warn_report("device %s is deprecated (%s)",
+                    type, dc->deprecation_reason);
+    }
+
     dev = DEVICE(object_new(type));
     if (!dev) {
         return NULL;

include/hw/qdev-core.h +1 -0

index a24d0dd..a352eaa 100644
@@ -105,6 +105,7 @@ typedef struct DeviceClass {
      */
     bool user_creatable;
     bool hotpluggable;
+    const char *deprecation_reason;
 
     /* callbacks */
     DeviceReset reset;

qdev-monitor.c +7 -0

index 802c18a..bbba2bc 100644
@@ -128,6 +128,9 @@ static void qdev_print_devinfo(DeviceClass *dc)
     if (!dc->user_creatable) {
         out_printf(", no-user");
     }
+    if (!dc->deprecation_reason) {
+        out_printf(", deprecated");
+    }
     out_printf("\n");
 }

@@ -579,6 +582,10 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
     if (!dc) {
         return NULL;
     }
+    if (dc->deprecation_reason) {
+        warn_report("device %s is deprecated (%s)",
+                    driver, dc->deprecation_reason);
+    }
 
     /* find bus */
     path = qemu_opt_get(opts, "bus");

Signature · 1 line

2.9.3

P J P

2018-10-25 10:54 UTC #2 lore raw

Re: [Qemu-devel] [PATCH 1/3] qdev: add deprecation_reason to DeviceClass

+-- On Thu, 25 Oct 2018, Gerd Hoffmann wrote --+
| Simliar to deprecated machine types.
| Print a warning when creating a deprecated device.
| Add deprecation notice to -device help.
| 
| TODO: add to intospection.

s/intospection/introspection ..?

| diff --git a/hw/core/qdev.c b/hw/core/qdev.c
| index 046d8f1..3b27a74 100644
| --- a/hw/core/qdev.c
| +++ b/hw/core/qdev.c
| @@ -133,11 +133,18 @@ DeviceState *qdev_create(BusState *bus, const char *name)
|  
|  DeviceState *qdev_try_create(BusState *bus, const char *type)
|  {
| +    DeviceClass *dc;
|      DeviceState *dev;
|  
| -    if (object_class_by_name(type) == NULL) {
| +    dc = DEVICE_CLASS(object_class_by_name(type));
| +    if (dc == NULL) {
|          return NULL;
|      }
| +    if (dc->deprecation_reason) {
| +        warn_report("device %s is deprecated (%s)",
| +                    type, dc->deprecation_reason);
| +    }
| +
|      dev = DEVICE(object_new(type));
|      if (!dev) {
|          return NULL;
| diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
| index a24d0dd..a352eaa 100644
| --- a/include/hw/qdev-core.h
| +++ b/include/hw/qdev-core.h
| @@ -105,6 +105,7 @@ typedef struct DeviceClass {
|       */
|      bool user_creatable;
|      bool hotpluggable;
| +    const char *deprecation_reason;
|  
|      /* callbacks */
|      DeviceReset reset;
| diff --git a/qdev-monitor.c b/qdev-monitor.c
| index 802c18a..bbba2bc 100644
| --- a/qdev-monitor.c
| +++ b/qdev-monitor.c
| @@ -128,6 +128,9 @@ static void qdev_print_devinfo(DeviceClass *dc)
|      if (!dc->user_creatable) {
|          out_printf(", no-user");
|      }
| +    if (!dc->deprecation_reason) {
| +        out_printf(", deprecated");
| +    }
|      out_printf("\n");
|  }
|  
| @@ -579,6 +582,10 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
|      if (!dc) {
|          return NULL;
|      }
| +    if (dc->deprecation_reason) {
| +        warn_report("device %s is deprecated (%s)",
| +                    driver, dc->deprecation_reason);
| +    }
|  
|      /* find bus */
|      path = qemu_opt_get(opts, "bus");
|

Looks good. Should 'deprecation_reason' be listed in qdev_device_help()?

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Philippe Mathieu-Daudé

Reviewed-by on v1 1/3

2018-10-25 11:12 UTC #3 lore raw

Re: [Qemu-devel] [PATCH 1/3] qdev: add deprecation_reason to DeviceClass

Hi Gerd,

Quoted 1 line · Gerd Hoffmann

Simliar to deprecated machine types.

"Similar"

Quoted 4 lines

Print a warning when creating a deprecated device. Add deprecation notice to -device help.
TODO: add to intospection.

"introspection"

Do we want the TODO in the git history?

Quoted 49 lines

---
  hw/core/qdev.c         | 9 ++++++++-
  include/hw/qdev-core.h | 1 +
  qdev-monitor.c         | 7 +++++++
  3 files changed, 16 insertions(+), 1 deletion(-)

hw/core/qdev.c +8 -1

index 046d8f1..3b27a74 100644
@@ -133,11 +133,18 @@ DeviceState *qdev_create(BusState *bus, const char *name)
 
  DeviceState *qdev_try_create(BusState *bus, const char *type)
  {
+    DeviceClass *dc;
      DeviceState *dev;
 
-    if (object_class_by_name(type) == NULL) {
+    dc = DEVICE_CLASS(object_class_by_name(type));
+    if (dc == NULL) {
          return NULL;
      }
+    if (dc->deprecation_reason) {
+        warn_report("device %s is deprecated (%s)",
+                    type, dc->deprecation_reason);
+    }
+
      dev = DEVICE(object_new(type));
      if (!dev) {
          return NULL;

include/hw/qdev-core.h +1 -0

index a24d0dd..a352eaa 100644
@@ -105,6 +105,7 @@ typedef struct DeviceClass {
       */
      bool user_creatable;
      bool hotpluggable;
+    const char *deprecation_reason;
 
      /* callbacks */
      DeviceReset reset;

qdev-monitor.c +0 -0

index 802c18a..bbba2bc 100644

qdev-monitor.c static void qdev_print_devinfo(DeviceClass *dc) · line 131

Quoted 4 lines

qdev-monitor.c +1 -0

@@ -128,6 +128,9 @@ static void qdev_print_devinfo(DeviceClass *dc)
      if (!dc->user_creatable) {
          out_printf(", no-user");
      }
+    if (!dc->deprecation_reason) {

This is the opposite condition:

       if (dc->deprecation_reason) {
           out_printf(", deprecated");

With it fixed:

Reviewed-by Philippe Mathieu-Daudé <[email protected]>

Quoted 17 lines

qdev-monitor.c +6 -0

+        out_printf(", deprecated");
+    }
      out_printf("\n");
  }

@@ -579,6 +582,10 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
      if (!dc) {
          return NULL;
      }
+    if (dc->deprecation_reason) {
+        warn_report("device %s is deprecated (%s)",
+                    driver, dc->deprecation_reason);
+    }
 
      /* find bus */
      path = qemu_opt_get(opts, "bus");

Gerd Hoffmann

author

2018-10-29 09:19 UTC #4 lore raw

Re: [Qemu-devel] [PATCH 1/3] qdev: add deprecation_reason to DeviceClass

Quoted 7 lines · Philippe Mathieu-Daudé

Hi Gerd,
Quoted 1 line · Gerd Hoffmann
Simliar to deprecated machine types.
"Similar"

Quoted 8 lines

Quoted 4 lines
Print a warning when creating a deprecated device. Add deprecation notice to -device help.
TODO: add to intospection.
"introspection"
Do we want the TODO in the git history?

No. It's RfC because of the missing introspection bits ;)

cheers,
Gerd

Gerd Hoffmann

author

2018-10-25 09:31 UTC #5 lore raw

PATCH 2/3 adlib: mark as insecure and deprecated.

We have a lovely, guest-triggerable buffer overflow in opl2 emulation.

Reproducer:

    outw(0xff60, 0x220);
    outw(0x1020, 0x220);
    outw(0xffb0, 0x220);
Result:
    Will overflow FM_OPL->AR_TABLE[] (see hw/audio/fmopl.[ch])

The specs google finds (http://www.symphoniae.com/Yamaha/YSC/YM3812.pdf)
are rather sparse, possibly incomplete (looks like scanned fax with a scrambled page at the end) and not really helpful in identifying which of the register writes sets some illegal value.

So, go tag the device as deprecated with a warning messge, to notify users and schedule it for removal according to our deprecation policy.

---
 hw/audio/adlib.c     | 1 +
 qemu-deprecated.texi | 4 ++++
 2 files changed, 5 insertions(+)

hw/audio/adlib.c +1 -0

index 97b876c..fb4a29c 100644
@@ -311,6 +311,7 @@ static void adlib_class_initfn (ObjectClass *klass, void *data)
     set_bit(DEVICE_CATEGORY_SOUND, dc->categories);
     dc->desc = ADLIB_DESC;
     dc->props = adlib_properties;
+    dc->deprecation_reason = "insecure, buffer overflow in opl2 emulation";
 }
 
 static const TypeInfo adlib_info = {

qemu-deprecated.texi +4 -0

index 11b870c..7951a4f 100644
@@ -116,6 +116,10 @@ The @option{[hub_id name]} parameter tuple of the 'hostfwd_add' and
 The ``ivshmem'' device type is replaced by either the ``ivshmem-plain''
 or ``ivshmem-doorbell`` device types.
 
+@subsection adlib (since 3.1)
+
+Has known buffer overflow.
+
 @section System emulator machines
 
 @subsection pc-0.10 and pc-0.11 (since 3.0)

Signature · 1 line

2.9.3

P J P

2018-10-25 10:56 UTC #6 lore raw

Re: [Qemu-devel] [PATCH 2/3] adlib: mark as insecure and deprecated.

+-- On Thu, 25 Oct 2018, Gerd Hoffmann wrote --+
| We have a lovely, guest-triggerable buffer overflow in opl2 emulation.
| 
| Reproducer:
|     outw(0xff60, 0x220);
|     outw(0x1020, 0x220);
|     outw(0xffb0, 0x220);
| Result:
|     Will overflow FM_OPL->AR_TABLE[] (see hw/audio/fmopl.[ch])

+ Reported-by: Wangjunqing <[email protected]>

| diff --git a/hw/audio/adlib.c b/hw/audio/adlib.c
| index 97b876c..fb4a29c 100644
| --- a/hw/audio/adlib.c
| +++ b/hw/audio/adlib.c
| @@ -311,6 +311,7 @@ static void adlib_class_initfn (ObjectClass *klass, void *data)
|      set_bit(DEVICE_CATEGORY_SOUND, dc->categories);
|      dc->desc = ADLIB_DESC;
|      dc->props = adlib_properties;
| +    dc->deprecation_reason = "insecure, buffer overflow in opl2 emulation";
|  }
|  
|  static const TypeInfo adlib_info = {
| diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
| index 11b870c..7951a4f 100644
| --- a/qemu-deprecated.texi
| +++ b/qemu-deprecated.texi
| @@ -116,6 +116,10 @@ The @option{[hub_id name]} parameter tuple of the 'hostfwd_add' and
|  The ``ivshmem'' device type is replaced by either the ``ivshmem-plain''
|  or ``ivshmem-doorbell`` device types.
|  
| +@subsection adlib (since 3.1)
| +
| +Has known buffer overflow.
| +
|  @section System emulator machines
|  
|  @subsection pc-0.10 and pc-0.11 (since 3.0)

Okay.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Daniel P. Berrangé

2018-10-25 20:40 UTC #7 lore raw

Re: [Qemu-devel] [libvirt] [PATCH 2/3] adlib: mark as insecure and deprecated.

Quoted 11 lines · P J P

+-- On Thu, 25 Oct 2018, Gerd Hoffmann wrote --+
| We have a lovely, guest-triggerable buffer overflow in opl2 emulation.
| 
| Reproducer:
|     outw(0xff60, 0x220);
|     outw(0x1020, 0x220);
|     outw(0xffb0, 0x220);
| Result:
|     Will overflow FM_OPL->AR_TABLE[] (see hw/audio/fmopl.[ch])

+ Reported-by: Wangjunqing <[email protected]>

So you have a CVE number for this ?

If so, it should be referenced in the commit message too, even if we can't fix the problem.

Quoted 15 lines

| diff --git a/hw/audio/adlib.c b/hw/audio/adlib.c
| index 97b876c..fb4a29c 100644
| --- a/hw/audio/adlib.c
| +++ b/hw/audio/adlib.c
| @@ -311,6 +311,7 @@ static void adlib_class_initfn (ObjectClass *klass, void *data)
|      set_bit(DEVICE_CATEGORY_SOUND, dc->categories);
|      dc->desc = ADLIB_DESC;
|      dc->props = adlib_properties;
| +    dc->deprecation_reason = "insecure, buffer overflow in opl2 emulation";
|  }
|  
|  static const TypeInfo adlib_info = {
| diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
| index 11b870c..7951a4f 100644
| --- a/qemu-deprecated.texi

Quoted 8 lines

| +++ b/qemu-deprecated.texi
| @@ -116,6 +116,10 @@ The @option{[hub_id name]} parameter tuple of the 'hostfwd_add' and
|  The ``ivshmem'' device type is replaced by either the ``ivshmem-plain''
|  or ``ivshmem-doorbell`` device types.
|  
| +@subsection adlib (since 3.1)
| +
| +Has known buffer overflow.

It would be good to give a recommendation to a better choice to replace its usage

Quoted 4 lines

| +
|  @section System emulator machines
|  
|  @subsection pc-0.10 and pc-0.11 (since 3.0)

Regards,
Daniel

Signature · 3 lines

|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

P J P

2018-10-26 07:09 UTC #8 lore raw

Re: [Qemu-devel] [libvirt] [PATCH 2/3] adlib: mark as insecure and deprecated.

+-- On Thu, 25 Oct 2018, Daniel P. Berrangé wrote --+
| On Thu, Oct 25, 2018 at 04:26:16PM +0530, P J P wrote:
| > +-- On Thu, 25 Oct 2018, Gerd Hoffmann wrote --+
| > | We have a lovely, guest-triggerable buffer overflow in opl2 emulation.
| > | 
| > | Reproducer:
| > |     outw(0xff60, 0x220);
| > |     outw(0x1020, 0x220);
| > |     outw(0xffb0, 0x220);
| > | Result:
| > |     Will overflow FM_OPL->AR_TABLE[] (see hw/audio/fmopl.[ch])
| > 
| > + Reported-by: Wangjunqing <[email protected]>
| 
| So you have a CVE number for this ?

No, since the adlib device is not used as much and is being deprecated, I'm not inclined to get one.

--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Daniel P. Berrangé

2018-10-26 09:54 UTC #9 lore raw

Re: [Qemu-devel] [libvirt] [PATCH 2/3] adlib: mark as insecure and deprecated.

Quoted 16 lines · P J P

+-- On Thu, 25 Oct 2018, Daniel P. Berrangé wrote --+
| On Thu, Oct 25, 2018 at 04:26:16PM +0530, P J P wrote:
| > +-- On Thu, 25 Oct 2018, Gerd Hoffmann wrote --+
| > | We have a lovely, guest-triggerable buffer overflow in opl2 emulation.
| > | 
| > | Reproducer:
| > |     outw(0xff60, 0x220);
| > |     outw(0x1020, 0x220);
| > |     outw(0xffb0, 0x220);
| > | Result:
| > |     Will overflow FM_OPL->AR_TABLE[] (see hw/audio/fmopl.[ch])
| > 
| > + Reported-by: Wangjunqing <[email protected]>
| 
| So you have a CVE number for this ?

Quoted 2 lines

No, since the adlib device is not used as much and is being deprecated, I'm not inclined to get one.

Any security issue that affects code in QEMU that is currently being shipped by distros should have a CVE.

Whether we intend to deprecate & delete it later should not be a factor because we are free to cancel the deprecation process at any time if we find a reason to keep the feature around.

Regards,
Daniel

Signature · 3 lines

|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

P J P

2018-10-26 12:35 UTC #10 lore raw

Re: [Qemu-devel] [libvirt] [PATCH 2/3] adlib: mark as insecure and deprecated.

+-- On Fri, 26 Oct 2018, Daniel P. Berrangé wrote --+
| > No, since the adlib device is not used as much and is being deprecated, I'm 
| > not inclined to get one.
| 
| Any security issue that affects code in QEMU that is currently being
| shipped by distros should have a CVE.
| 
| Whether we intend to deprecate & delete it later should not be a factor
| because we are free to cancel the deprecation process at any time if we
| find a reason to keep the feature around.

Okay, will follow up with a CVE process. Thank you. --
Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Philippe Mathieu-Daudé

Reviewed-by on v1 2/3

2018-10-25 11:33 UTC #11 lore raw

Re: [Qemu-devel] [PATCH 2/3] adlib: mark as insecure and deprecated.

Quoted 9 lines · Gerd Hoffmann

We have a lovely, guest-triggerable buffer overflow in opl2 emulation.
Reproducer:
     outw(0xff60, 0x220);
     outw(0x1020, 0x220);
     outw(0xffb0, 0x220);
Result:
     Will overflow FM_OPL->AR_TABLE[] (see hw/audio/fmopl.[ch])

Quoted 6 lines

The specs google finds (http://www.symphoniae.com/Yamaha/YSC/YM3812.pdf)
are rather sparse, possibly incomplete (looks like scanned fax with a scrambled page at the end) and not really helpful in identifying which of the register writes sets some illegal value.
So, go tag the device as deprecated with a warning messge, to notify

"warning message"

Quoted 26 lines

users and schedule it for removal according to our deprecation policy.

---
  hw/audio/adlib.c     | 1 +
  qemu-deprecated.texi | 4 ++++
  2 files changed, 5 insertions(+)

hw/audio/adlib.c +1 -0

index 97b876c..fb4a29c 100644
@@ -311,6 +311,7 @@ static void adlib_class_initfn (ObjectClass *klass, void *data)
      set_bit(DEVICE_CATEGORY_SOUND, dc->categories);
      dc->desc = ADLIB_DESC;
      dc->props = adlib_properties;
+    dc->deprecation_reason = "insecure, buffer overflow in opl2 emulation";
  }
 
  static const TypeInfo adlib_info = {

qemu-deprecated.texi +0 -0

index 11b870c..7951a4f 100644

qemu-deprecated.texi The @option{[hub_id name]} parameter tuple of the 'hostfwd_add' and · line 125

Quoted 10 lines

qemu-deprecated.texi +4 -0

@@ -116,6 +116,10 @@ The @option{[hub_id name]} parameter tuple of the 'hostfwd_add' and
  The ``ivshmem'' device type is replaced by either the ``ivshmem-plain''
  or ``ivshmem-doorbell`` device types.
 
+@subsection adlib (since 3.1)
+
+Has known buffer overflow.
+
  @section System emulator machines
 
  @subsection pc-0.10 and pc-0.11 (since 3.0)

Reviewed-by Philippe Mathieu-Daudé <[email protected]>

Thomas Huth

Reviewed-by on v1 2/3

2018-10-25 20:35 UTC #12 lore raw

Re: [Qemu-devel] [PATCH 2/3] adlib: mark as insecure and deprecated.

Quoted 41 lines · Gerd Hoffmann

We have a lovely, guest-triggerable buffer overflow in opl2 emulation.
Reproducer:
    outw(0xff60, 0x220);
    outw(0x1020, 0x220);
    outw(0xffb0, 0x220);
Result:
    Will overflow FM_OPL->AR_TABLE[] (see hw/audio/fmopl.[ch])
The specs google finds (http://www.symphoniae.com/Yamaha/YSC/YM3812.pdf)
are rather sparse, possibly incomplete (looks like scanned fax with a scrambled page at the end) and not really helpful in identifying which of the register writes sets some illegal value.
So, go tag the device as deprecated with a warning messge, to notify users and schedule it for removal according to our deprecation policy.
Signed-off-by Gerd Hoffmann <[email protected]>
---
 hw/audio/adlib.c     | 1 +
 qemu-deprecated.texi | 4 ++++
 2 files changed, 5 insertions(+)
hw/audio/adlib.c +1 -0
index 97b876c..fb4a29c 100644
@@ -311,6 +311,7 @@ static void adlib_class_initfn (ObjectClass *klass, void *data)
     set_bit(DEVICE_CATEGORY_SOUND, dc->categories);
     dc->desc = ADLIB_DESC;
     dc->props = adlib_properties;
+    dc->deprecation_reason = "insecure, buffer overflow in opl2 emulation";
 }
 
 static const TypeInfo adlib_info = {
qemu-deprecated.texi +0 -0
index 11b870c..7951a4f 100644

qemu-deprecated.texi The @option{[hub_id name]} parameter tuple of the 'hostfwd_add' and · line 125

Quoted 10 lines

qemu-deprecated.texi +4 -0

@@ -116,6 +116,10 @@ The @option{[hub_id name]} parameter tuple of the 'hostfwd_add' and
 The ``ivshmem'' device type is replaced by either the ``ivshmem-plain''
 or ``ivshmem-doorbell`` device types.
 
+@subsection adlib (since 3.1)
+
+Has known buffer overflow.
+
 @section System emulator machines
 
 @subsection pc-0.10 and pc-0.11 (since 3.0)

Any chance you could maybe at least add an assert() to the affected code so that it crashes instead of silently overflowing the buffer?

Anyway, with the "messge" typo fixed:

Reviewed-by Thomas Huth <[email protected]>

Paolo Bonzini

2018-10-26 08:48 UTC #13 lore raw

Re: [Qemu-devel] [PATCH 2/3] adlib: mark as insecure and deprecated.

Quoted 8 lines · Gerd Hoffmann

We have a lovely, guest-triggerable buffer overflow in opl2 emulation.
Reproducer:
    outw(0xff60, 0x220);
    outw(0x1020, 0x220);
    outw(0xffb0, 0x220);
Result:
    Will overflow FM_OPL->AR_TABLE[] (see hw/audio/fmopl.[ch])

I am dumb and I don't understand. In set_ar_dr you get

	v = 0xff
	ar = 15
	dr = 15

and OPL->AR_TABLE[60] is accessed. The size of the array is 75, which seems to be actually 14 more than required. Likewise OPL->DR_TABLE[60] is accessed.

The next accesses use SLOT->ksr which is 0 so it's fine too.

Paolo

P J P

2018-10-26 09:35 UTC #14 lore raw

Re: [Qemu-devel] [PATCH 2/3] adlib: mark as insecure and deprecated.

+-- On Fri, 26 Oct 2018, Paolo Bonzini wrote --+
| I am dumb and I don't understand.  In set_ar_dr you get
| 
| 	v = 0xff
| 	ar = 15
| 	dr = 15
| 
| and OPL->AR_TABLE[60] is accessed.  The size of the array is 75, which
| seems to be actually 14 more than required.  Likewise OPL->DR_TABLE[60]
| is accessed.
| 
| The next accesses use SLOT->ksr which is 0 so it's fine too.

In set_ar_dr

  SLOT->AR = ar ? &OPL->AR_TABLE[ar<<2] : RATE_0;

SLOT->AR is set to point to OPL->DR_TABLE[60] and while so if s->ksr is set to 15, in CALC_FCSLOT()

  SLOT->evsa = SLOT->AR[ksr];  <= accesses OPL->AR_TABLE[60 + 15];

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Paolo Bonzini

2018-10-26 10:02 UTC #15 lore raw

Re: [Qemu-devel] [PATCH 2/3] adlib: mark as insecure and deprecated.

Quoted 13 lines · P J P

+-- On Fri, 26 Oct 2018, Paolo Bonzini wrote --+
| I am dumb and I don't understand.  In set_ar_dr you get
| 
| 	v = 0xff
| 	ar = 15
| 	dr = 15
| 
| and OPL->AR_TABLE[60] is accessed.  The size of the array is 75, which
| seems to be actually 14 more than required.  Likewise OPL->DR_TABLE[60]
| is accessed.
| 
| The next accesses use SLOT->ksr which is 0 so it's fine too.

Quoted 8 lines

In set_ar_dr
  SLOT->AR = ar ? &OPL->AR_TABLE[ar<<2] : RATE_0;
SLOT->AR is set to point to OPL->DR_TABLE[60] and while so if s->ksr is set to 15, in CALC_FCSLOT()
  SLOT->evsa = SLOT->AR[ksr];  <= accesses OPL->AR_TABLE[60 + 15];

Oh, thanks! I said I was dumb. :) So the fix is just this:

hw/audio/fmopl.h +2 -2

index e7e578a48e..7199afaa3c 100644
@@ -72,8 +72,8 @@ typedef struct fm_opl_f {
 	/* Rhythm sention */
 	uint8_t rhythm;		/* Rhythm mode , key flag */
 	/* time tables */
-	int32_t AR_TABLE[75];	/* atttack rate tables */
-	int32_t DR_TABLE[75];	/* decay rate tables   */
+	int32_t AR_TABLE[76];	/* atttack rate tables */
+	int32_t DR_TABLE[76];	/* decay rate tables   */
 	uint32_t FN_TABLE[1024];  /* fnumber -> increment counter */
 	/* LFO */
 	int32_t *ams_table;

and init_timetables will just fill it with the right value? (I checked against another implementation at http://opl3.cozendey.com/).

Thanks,

Paolo

P J P

2018-10-26 11:53 UTC #16 lore raw

Re: [Qemu-devel] [PATCH 2/3] adlib: mark as insecure and deprecated.

+-- On Fri, 26 Oct 2018, Paolo Bonzini wrote --+
| Oh, thanks!  I said I was dumb. :)  So the fix is just this:
| 
| diff --git a/hw/audio/fmopl.h b/hw/audio/fmopl.h
| index e7e578a48e..7199afaa3c 100644
| --- a/hw/audio/fmopl.h
| +++ b/hw/audio/fmopl.h
| @@ -72,8 +72,8 @@ typedef struct fm_opl_f {
|  	/* Rhythm sention */
|  	uint8_t rhythm;		/* Rhythm mode , key flag */
|  	/* time tables */
| -	int32_t AR_TABLE[75];	/* atttack rate tables */
| -	int32_t DR_TABLE[75];	/* decay rate tables   */
| +	int32_t AR_TABLE[76];	/* atttack rate tables */
| +	int32_t DR_TABLE[76];	/* decay rate tables   */
|  	uint32_t FN_TABLE[1024];  /* fnumber -> increment counter */
|  	/* LFO */
|  	int32_t *ams_table;
| 
| and init_timetables will just fill it with the right value?  (I checked
| against another implementation at http://opl3.cozendey.com/).

Gerd has proposed to a patch to deprecate adlib, as it's not used as much. IMO deprecation is better option. But if that is not happening, above seems good.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Gerd Hoffmann

author

2018-10-29 09:05 UTC #17 lore raw

Re: [Qemu-devel] [PATCH 2/3] adlib: mark as insecure and deprecated.

Quoted 22 lines · P J P

+-- On Fri, 26 Oct 2018, Paolo Bonzini wrote --+
| Oh, thanks!  I said I was dumb. :)  So the fix is just this:
| 
| diff --git a/hw/audio/fmopl.h b/hw/audio/fmopl.h
| index e7e578a48e..7199afaa3c 100644
| --- a/hw/audio/fmopl.h
| +++ b/hw/audio/fmopl.h
| @@ -72,8 +72,8 @@ typedef struct fm_opl_f {
|  	/* Rhythm sention */
|  	uint8_t rhythm;		/* Rhythm mode , key flag */
|  	/* time tables */
| -	int32_t AR_TABLE[75];	/* atttack rate tables */
| -	int32_t DR_TABLE[75];	/* decay rate tables   */
| +	int32_t AR_TABLE[76];	/* atttack rate tables */
| +	int32_t DR_TABLE[76];	/* decay rate tables   */
|  	uint32_t FN_TABLE[1024];  /* fnumber -> increment counter */
|  	/* LFO */
|  	int32_t *ams_table;
| 
| and init_timetables will just fill it with the right value?  (I checked
| against another implementation at http://opl3.cozendey.com/).

Quoted 2 lines

Gerd has proposed to a patch to deprecate adlib, as it's not used as much. IMO deprecation is better option. But if that is not happening, above seems good.

I think we can actually do both.

cheers,
Gerd

Gerd Hoffmann

author

2018-10-25 09:31 UTC #18 lore raw

PATCH 3/3 cirrus: mark as deprecated

While being at it deprecate cirrus too.

Reason (short version): use stdvga instead.
Verbose version:

    https://www.kraxel.org/blog/2014/10/qemu-using-cirrus-considered-harmful

---
 hw/display/cirrus_vga.c     | 2 ++
 hw/display/cirrus_vga_isa.c | 2 ++
 qemu-deprecated.texi        | 4 ++++
 3 files changed, 8 insertions(+)

hw/display/cirrus_vga.c +2 -0

index d9b854d..2f16ba9 100644
@@ -3024,6 +3024,8 @@ static void cirrus_vga_class_init(ObjectClass *klass, void *data)
     dc->vmsd = &vmstate_pci_cirrus_vga;
     dc->props = pci_vga_cirrus_properties;
     dc->hotpluggable = false;
+    dc->deprecation_reason =
+        "https://www.kraxel.org/blog/2014/10/qemu-using-cirrus-considered-harmful";
 }
 
 static const TypeInfo cirrus_vga_info = {

hw/display/cirrus_vga_isa.c +2 -0

index fa10b74..c2d853c 100644
@@ -81,6 +81,8 @@ static void isa_cirrus_vga_class_init(ObjectClass *klass, void *data)
     dc->realize = isa_cirrus_vga_realizefn;
     dc->props = isa_cirrus_vga_properties;
     set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
+    dc->deprecation_reason =
+        "https://www.kraxel.org/blog/2014/10/qemu-using-cirrus-considered-harmful";
 }
 
 static const TypeInfo isa_cirrus_vga_info = {

qemu-deprecated.texi +4 -0

index 7951a4f..1b1d434 100644
@@ -120,6 +120,10 @@ or ``ivshmem-doorbell`` device types.
 
 Has known buffer overflow.
 
+@subsection cirrus (since 3.1)
+
+Use stdvga instead (-vga std or -device VGA).
+
 @section System emulator machines
 
 @subsection pc-0.10 and pc-0.11 (since 3.0)

Signature · 1 line

2.9.3

P J P

Reviewed-by Prasad J Pandit on v1 3/3

2018-10-25 10:59 UTC #19 lore raw

Re: [Qemu-devel] [PATCH 3/3] cirrus: mark as deprecated

+-- On Thu, 25 Oct 2018, Gerd Hoffmann wrote --+
| While being at it deprecate cirrus too.
| 
| Reason (short version): use stdvga instead.
| Verbose version:
|     https://www.kraxel.org/blog/2014/10/qemu-using-cirrus-considered-harmful
| 
| Signed-off-by: Gerd Hoffmann <[email protected]>
| ---
|  hw/display/cirrus_vga.c     | 2 ++
|  hw/display/cirrus_vga_isa.c | 2 ++
|  qemu-deprecated.texi        | 4 ++++
|  3 files changed, 8 insertions(+)
| 
| diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
| index d9b854d..2f16ba9 100644
| --- a/hw/display/cirrus_vga.c
| +++ b/hw/display/cirrus_vga.c
| @@ -3024,6 +3024,8 @@ static void cirrus_vga_class_init(ObjectClass *klass, void *data)
|      dc->vmsd = &vmstate_pci_cirrus_vga;
|      dc->props = pci_vga_cirrus_properties;
|      dc->hotpluggable = false;
| +    dc->deprecation_reason =
| +        "https://www.kraxel.org/blog/2014/10/qemu-using-cirrus-considered-harmful";
|  }
|  
|  static const TypeInfo cirrus_vga_info = {
| diff --git a/hw/display/cirrus_vga_isa.c b/hw/display/cirrus_vga_isa.c
| index fa10b74..c2d853c 100644
| --- a/hw/display/cirrus_vga_isa.c
| +++ b/hw/display/cirrus_vga_isa.c
| @@ -81,6 +81,8 @@ static void isa_cirrus_vga_class_init(ObjectClass *klass, void *data)
|      dc->realize = isa_cirrus_vga_realizefn;
|      dc->props = isa_cirrus_vga_properties;
|      set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
| +    dc->deprecation_reason =
| +        "https://www.kraxel.org/blog/2014/10/qemu-using-cirrus-considered-harmful";
|  }
|  
|  static const TypeInfo isa_cirrus_vga_info = {
| diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
| index 7951a4f..1b1d434 100644
| --- a/qemu-deprecated.texi
| +++ b/qemu-deprecated.texi
| @@ -120,6 +120,10 @@ or ``ivshmem-doorbell`` device types.
|  
|  Has known buffer overflow.
|  
| +@subsection cirrus (since 3.1)
| +
| +Use stdvga instead (-vga std or -device VGA).
| +
|  @section System emulator machines
|  
|  @subsection pc-0.10 and pc-0.11 (since 3.0)
|

Reviewed-by Prasad J Pandit <[email protected]>

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Philippe Mathieu-Daudé

Reviewed-by on v1 3/3

2018-10-25 11:33 UTC #20 lore raw

Re: [Qemu-devel] [PATCH 3/3] cirrus: mark as deprecated

Quoted 18 lines · Gerd Hoffmann

While being at it deprecate cirrus too.
Reason (short version): use stdvga instead.
Verbose version:
     https://www.kraxel.org/blog/2014/10/qemu-using-cirrus-considered-harmful
Signed-off-by Gerd Hoffmann <[email protected]>
---
  hw/display/cirrus_vga.c     | 2 ++
  hw/display/cirrus_vga_isa.c | 2 ++
  qemu-deprecated.texi        | 4 ++++
  3 files changed, 8 insertions(+)
hw/display/cirrus_vga.c +0 -0
index d9b854d..2f16ba9 100644

hw/display/cirrus_vga.c static void cirrus_vga_class_init(ObjectClass *klass, void *data) · line 3028

Quoted 5 lines

hw/display/cirrus_vga.c +2 -0

@@ -3024,6 +3024,8 @@ static void cirrus_vga_class_init(ObjectClass *klass, void *data)
      dc->vmsd = &vmstate_pci_cirrus_vga;
      dc->props = pci_vga_cirrus_properties;
      dc->hotpluggable = false;
+    dc->deprecation_reason =
+        "https://www.kraxel.org/blog/2014/10/qemu-using-cirrus-considered-harmful";

We should propably add an archived copy of this post to www.qemu.org.

Reviewed-by Philippe Mathieu-Daudé <[email protected]>

Quoted 32 lines

hw/display/cirrus_vga.c +0 -0

  }
 
  static const TypeInfo cirrus_vga_info = {

hw/display/cirrus_vga_isa.c +2 -0

index fa10b74..c2d853c 100644
@@ -81,6 +81,8 @@ static void isa_cirrus_vga_class_init(ObjectClass *klass, void *data)
      dc->realize = isa_cirrus_vga_realizefn;
      dc->props = isa_cirrus_vga_properties;
      set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
+    dc->deprecation_reason =
+        "https://www.kraxel.org/blog/2014/10/qemu-using-cirrus-considered-harmful";
  }
 
  static const TypeInfo isa_cirrus_vga_info = {

qemu-deprecated.texi +4 -0

index 7951a4f..1b1d434 100644
@@ -120,6 +120,10 @@ or ``ivshmem-doorbell`` device types.
 
  Has known buffer overflow.
 
+@subsection cirrus (since 3.1)
+
+Use stdvga instead (-vga std or -device VGA).
+
  @section System emulator machines
 
  @subsection pc-0.10 and pc-0.11 (since 3.0)

Thomas Huth

Reviewed-by on v1 3/3

2018-10-25 20:35 UTC #21 lore raw

Re: [Qemu-devel] [PATCH 3/3] cirrus: mark as deprecated

Quoted 45 lines · Gerd Hoffmann

While being at it deprecate cirrus too.

Reason (short version): use stdvga instead.
Verbose version:

    https://www.kraxel.org/blog/2014/10/qemu-using-cirrus-considered-harmful

---
 hw/display/cirrus_vga.c     | 2 ++
 hw/display/cirrus_vga_isa.c | 2 ++
 qemu-deprecated.texi        | 4 ++++
 3 files changed, 8 insertions(+)

hw/display/cirrus_vga.c +2 -0

index d9b854d..2f16ba9 100644
@@ -3024,6 +3024,8 @@ static void cirrus_vga_class_init(ObjectClass *klass, void *data)
     dc->vmsd = &vmstate_pci_cirrus_vga;
     dc->props = pci_vga_cirrus_properties;
     dc->hotpluggable = false;
+    dc->deprecation_reason =
+        "https://www.kraxel.org/blog/2014/10/qemu-using-cirrus-considered-harmful";
 }
 
 static const TypeInfo cirrus_vga_info = {

hw/display/cirrus_vga_isa.c +2 -0

index fa10b74..c2d853c 100644
@@ -81,6 +81,8 @@ static void isa_cirrus_vga_class_init(ObjectClass *klass, void *data)
     dc->realize = isa_cirrus_vga_realizefn;
     dc->props = isa_cirrus_vga_properties;
     set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
+    dc->deprecation_reason =
+        "https://www.kraxel.org/blog/2014/10/qemu-using-cirrus-considered-harmful";
 }
 
 static const TypeInfo isa_cirrus_vga_info = {

qemu-deprecated.texi +0 -0

index 7951a4f..1b1d434 100644

qemu-deprecated.texi or ``ivshmem-doorbell`` device types. · line 129

Quoted 10 lines

qemu-deprecated.texi +4 -0

@@ -120,6 +120,10 @@ or ``ivshmem-doorbell`` device types.
 
 Has known buffer overflow.
 
+@subsection cirrus (since 3.1)
+
+Use stdvga instead (-vga std or -device VGA).
+
 @section System emulator machines
 
 @subsection pc-0.10 and pc-0.11 (since 3.0)

Reviewed-by Thomas Huth <[email protected]>

Daniel P. Berrangé

2018-10-25 20:38 UTC #22 lore raw

Re: [Qemu-devel] [PATCH 3/3] cirrus: mark as deprecated

Quoted 5 lines · Gerd Hoffmann

While being at it deprecate cirrus too.
Reason (short version): use stdvga instead.
Verbose version:
    https://www.kraxel.org/blog/2014/10/qemu-using-cirrus-considered-harmful

Every single one of my guests is using cirrus. This wasn't an explicit choice on my part, so I believe it is being used as the default in virt-install.

I don't debate the points in the blog post above that stdvga is a better choice, but I don't think that's enough to justify deprecating cirrus at this point in time, because when it then gets deleted it will break way too many existing deployments.

We need to socialize info in that blog post above more widely and especially ensure that apps are not using that by default. I don't see it being viable to formally deprecate it in QEMU any time soon though given existing usage.

Quoted 53 lines

https://qemu.weilnetz.de/doc/qemu-doc.html#Deprecated-features

---
 hw/display/cirrus_vga.c     | 2 ++
 hw/display/cirrus_vga_isa.c | 2 ++
 qemu-deprecated.texi        | 4 ++++
 3 files changed, 8 insertions(+)

hw/display/cirrus_vga.c +2 -0

index d9b854d..2f16ba9 100644
@@ -3024,6 +3024,8 @@ static void cirrus_vga_class_init(ObjectClass *klass, void *data)
     dc->vmsd = &vmstate_pci_cirrus_vga;
     dc->props = pci_vga_cirrus_properties;
     dc->hotpluggable = false;
+    dc->deprecation_reason =
+        "https://www.kraxel.org/blog/2014/10/qemu-using-cirrus-considered-harmful";
 }
 
 static const TypeInfo cirrus_vga_info = {

hw/display/cirrus_vga_isa.c +2 -0

index fa10b74..c2d853c 100644
@@ -81,6 +81,8 @@ static void isa_cirrus_vga_class_init(ObjectClass *klass, void *data)
     dc->realize = isa_cirrus_vga_realizefn;
     dc->props = isa_cirrus_vga_properties;
     set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
+    dc->deprecation_reason =
+        "https://www.kraxel.org/blog/2014/10/qemu-using-cirrus-considered-harmful";
 }
 
 static const TypeInfo isa_cirrus_vga_info = {

qemu-deprecated.texi +4 -0

index 7951a4f..1b1d434 100644
@@ -120,6 +120,10 @@ or ``ivshmem-doorbell`` device types.
 
 Has known buffer overflow.
 
+@subsection cirrus (since 3.1)
+
+Use stdvga instead (-vga std or -device VGA).
+
 @section System emulator machines
 
 @subsection pc-0.10 and pc-0.11 (since 3.0)

Signature · 1 line

2.9.3

Regards,
Daniel

Signature · 3 lines

|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

P J P

2018-10-26 07:04 UTC #23 lore raw

Re: [Qemu-devel] [PATCH 3/3] cirrus: mark as deprecated

Hello Dan, all

+-- On Thu, 25 Oct 2018, Daniel P. Berrangé wrote --+
| On Thu, Oct 25, 2018 at 10:52:56AM +0200, Gerd Hoffmann wrote:
| > While being at it deprecate cirrus too.
| > 
| > Reason (short version): use stdvga instead.
| > Verbose version:
| >     https://www.kraxel.org/blog/2014/10/qemu-using-cirrus-considered-harmful
| 
| 
| I don't debate the points in the blog post above that stdvga is a
| better choice, but I don't think that's enough to justify deprecating
| cirrus at this point in time, because when it then gets deleted it
| will break way too many existing deployments.
| 
| We need to socialize info in that blog post above more widely and
| especially ensure that apps are not using that by default. I don't
| see it being viable to formally deprecate it in QEMU any time soon
| though given existing usage.

To note, IMO there are other devices/sources in QEMU which are potential candidates for deprecation, similar to adlib etc. It'll help if we could device a process to deprecate/remove such code base. Other than maintenance it invariably also becomes source of security issues.

Ex.(similar to Fedora) we could announce such candidate on qemu-devel list and after review over a period of say a month, candidate will be deprecated/expunged. (thinking aloud)

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Daniel P. Berrangé

2018-10-26 09:42 UTC #24 lore raw

Re: [Qemu-devel] [PATCH 3/3] cirrus: mark as deprecated

Quoted 21 lines · P J P

Hello Dan, all

+-- On Thu, 25 Oct 2018, Daniel P. Berrangé wrote --+
| On Thu, Oct 25, 2018 at 10:52:56AM +0200, Gerd Hoffmann wrote:
| > While being at it deprecate cirrus too.
| > 
| > Reason (short version): use stdvga instead.
| > Verbose version:
| >     https://www.kraxel.org/blog/2014/10/qemu-using-cirrus-considered-harmful
| 
| 
| I don't debate the points in the blog post above that stdvga is a
| better choice, but I don't think that's enough to justify deprecating
| cirrus at this point in time, because when it then gets deleted it
| will break way too many existing deployments.
| 
| We need to socialize info in that blog post above more widely and
| especially ensure that apps are not using that by default. I don't
| see it being viable to formally deprecate it in QEMU any time soon
| though given existing usage.

Quoted 8 lines

To note, IMO there are other devices/sources in QEMU which are potential candidates for deprecation, similar to adlib etc. It'll help if we could device a process to deprecate/remove such code base. Other than maintenance it invariably also becomes source of security issues.
Ex.(similar to Fedora) we could announce such candidate on qemu-devel list and after review over a period of say a month, candidate will be deprecated/expunged. (thinking aloud)

QEMU has a deprecation process:

Most of the stuff deprecated is CLI args / monitor commands, etc where mgmt apps just adjust the way they are calling QEMU, so end user's VMs are largely not impacted.

Deprecating a device type that is widely used is not desirable because that will cause breakage of existing guests. Distros are free to disable devices in their builds if they want to reduce the scope for CVEs in packages they maintain, but again they should think carefully about how many users they are going to break by doing so.

Regards,
Daniel

Signature · 3 lines

|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Daniel P. Berrangé

2018-10-26 10:00 UTC #25 lore raw

Re: [Qemu-devel] [PATCH 3/3] cirrus: mark as deprecated

Quoted 39 lines · Daniel P. Berrangé

Quoted 21 lines · P J P
Hello Dan, all
+-- On Thu, 25 Oct 2018, Daniel P. Berrangé wrote --+
| On Thu, Oct 25, 2018 at 10:52:56AM +0200, Gerd Hoffmann wrote:
| > While being at it deprecate cirrus too.
| > 
| > Reason (short version): use stdvga instead.
| > Verbose version:
| >     https://www.kraxel.org/blog/2014/10/qemu-using-cirrus-considered-harmful
| 
| 
| I don't debate the points in the blog post above that stdvga is a
| better choice, but I don't think that's enough to justify deprecating
| cirrus at this point in time, because when it then gets deleted it
| will break way too many existing deployments.
| 
| We need to socialize info in that blog post above more widely and
| especially ensure that apps are not using that by default. I don't
| see it being viable to formally deprecate it in QEMU any time soon
| though given existing usage.
Quoted 8 lines
To note, IMO there are other devices/sources in QEMU which are potential candidates for deprecation, similar to adlib etc. It'll help if we could device a process to deprecate/remove such code base. Other than maintenance it invariably also becomes source of security issues.
Ex.(similar to Fedora) we could announce such candidate on qemu-devel list and after review over a period of say a month, candidate will be deprecated/expunged. (thinking aloud)
QEMU has a deprecation process:
https://qemu.weilnetz.de/doc/qemu-doc.html#Deprecated-features
Most of the stuff deprecated is CLI args / monitor commands, etc where mgmt apps just adjust the way they are calling QEMU, so end user's VMs are largely not impacted.

Quoted 5 lines

Deprecating a device type that is widely used is not desirable because that will cause breakage of existing guests. Distros are free to disable devices in their builds if they want to reduce the scope for CVEs in packages they maintain, but again they should think carefully about how many users they are going to break by doing so.

I should also say that QEMU as an upstream project has multiple goals. Running KVM guests with modern PV hardware is only one of them, albeit a widely used one. Being able to run old legacy OS with old hardware, and running arbitrary embedded boards/devices with emulation are both use cases that QEMU project aims to address. To eliminate all the old "crufty" device emulation in name of improving security for KVM, would be to eliminate core use cases of the project. THis is why we're trying to persue the direction of making it easier for vendors to disable features and devices they don't wish to support & thus limit their downstream CVE exposure.

Regards,
Daniel

Signature · 3 lines

|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Paolo Bonzini

2018-10-26 10:03 UTC #26 lore raw

Re: [Qemu-devel] [PATCH 3/3] cirrus: mark as deprecated

Quoted 10 lines · Daniel P. Berrangé

I should also say that QEMU as an upstream project has multiple goals. Running KVM guests with modern PV hardware is only one of them, albeit a widely used one. Being able to run old legacy OS with old hardware, and running arbitrary embedded boards/devices with emulation are both use cases that QEMU project aims to address. To eliminate all the old "crufty" device emulation in name of improving security for KVM, would be to eliminate core use cases of the project. THis is why we're trying to persue the direction of making it easier for vendors to disable features and devices they don't wish to support & thus limit their downstream CVE exposure.

Indeed. If we had to deprecate a feature just because it had an off-by-one bug, no C program would grow beyond 1000 lines of code...

Paolo

Daniel P. Berrangé

2018-10-26 14:15 UTC #27 lore raw

Re: [Qemu-devel] [PATCH 3/3] cirrus: mark as deprecated

Quoted 12 lines · Paolo Bonzini

Quoted 10 lines · Daniel P. Berrangé
I should also say that QEMU as an upstream project has multiple goals. Running KVM guests with modern PV hardware is only one of them, albeit a widely used one. Being able to run old legacy OS with old hardware, and running arbitrary embedded boards/devices with emulation are both use cases that QEMU project aims to address. To eliminate all the old "crufty" device emulation in name of improving security for KVM, would be to eliminate core use cases of the project. THis is why we're trying to persue the direction of making it easier for vendors to disable features and devices they don't wish to support & thus limit their downstream CVE exposure.

Quoted 2 lines

Indeed. If we had to deprecate a feature just because it had an off-by-one bug, no C program would grow beyond 1000 lines of code...

One thing we should do, however, is to make it clear which of the device models we consider secure, and which we consider only usable in a friendly guest environment, as we have very different code maintainership & quality standards for different parts of QEMU.

Essentially virtio devices, and then only a handful of the emulated devices are things we consider suitable for usage in secure envs. Likewise for machine types probably.

Regards,
Daniel

Signature · 3 lines

|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

P J P

2018-10-26 18:56 UTC #28 lore raw

Re: [Qemu-devel] [PATCH 3/3] cirrus: mark as deprecated

+-- On Fri, 26 Oct 2018, Daniel P. Berrangé wrote --+
| ... 
| One thing we should do, however, is to make it clear which of the
| device models we consider secure, and which we consider only usable
| in a friendly guest environment, as we have very different code
| maintainership & quality standards for different parts of QEMU.
| 
| Essentially virtio devices, and then only a handful of the emulated
| devices are things we consider suitable for usage in secure envs.
| Likewise for machine types probably.

True, +1.

It did come up in another thread. It'll surely be helpful to list these professional and friendly components. 'Professional' being production ready and thus security relevant. And 'Friendly' being experimental or not suitable for production usage. Maybe like staging drivers in the kernel tree. They are available for use but not considered production ready and thus are not security relevant.

To be clear, irrespective of professional or friendly, we strive to fix every single issue that is found and/or reported. Only difference is, professional ones are tracked by a CVE ID and friendly ones are fixed as bug fixes, not tracked by CVE ID.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Dr. David Alan Gilbert

2018-10-26 10:41 UTC #29 lore raw

Re: [Qemu-devel] [PATCH 3/3] cirrus: mark as deprecated

Quoted 39 lines · * Daniel P. Berrangé ([email protected]) wrote:

Quoted 21 lines · P J P
Hello Dan, all
+-- On Thu, 25 Oct 2018, Daniel P. Berrangé wrote --+
| On Thu, Oct 25, 2018 at 10:52:56AM +0200, Gerd Hoffmann wrote:
| > While being at it deprecate cirrus too.
| > 
| > Reason (short version): use stdvga instead.
| > Verbose version:
| >     https://www.kraxel.org/blog/2014/10/qemu-using-cirrus-considered-harmful
| 
| 
| I don't debate the points in the blog post above that stdvga is a
| better choice, but I don't think that's enough to justify deprecating
| cirrus at this point in time, because when it then gets deleted it
| will break way too many existing deployments.
| 
| We need to socialize info in that blog post above more widely and
| especially ensure that apps are not using that by default. I don't
| see it being viable to formally deprecate it in QEMU any time soon
| though given existing usage.
Quoted 8 lines
To note, IMO there are other devices/sources in QEMU which are potential candidates for deprecation, similar to adlib etc. It'll help if we could device a process to deprecate/remove such code base. Other than maintenance it invariably also becomes source of security issues.
Ex.(similar to Fedora) we could announce such candidate on qemu-devel list and after review over a period of say a month, candidate will be deprecated/expunged. (thinking aloud)
QEMU has a deprecation process:
https://qemu.weilnetz.de/doc/qemu-doc.html#Deprecated-features
Most of the stuff deprecated is CLI args / monitor commands, etc where mgmt apps just adjust the way they are calling QEMU, so end user's VMs are largely not impacted.

Quoted 5 lines

Deprecating a device type that is widely used is not desirable because that will cause breakage of existing guests. Distros are free to disable devices in their builds if they want to reduce the scope for CVEs in packages they maintain, but again they should think carefully about how many users they are going to break by doing so.

I'm a bit mixed here; until recent guest kernels I've always treated Cirrus as the 'safe' one that's likely to work on anything - so losing it worries me. Having said that, I understand why we want to deprecate it; but we should give a much longer deprecation warning - a few years? I don't see any harm warning that it's deprecated and you really should try not to use it.

Dave

Quoted 7 lines

Regards,
Daniel

Signature · 3 lines

|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK

Christian Borntraeger

2018-10-26 13:26 UTC #30 lore raw

Re: [Qemu-devel] [libvirt] [PATCH 3/3] cirrus: mark as deprecated

Quoted 39 lines · Daniel P. Berrangé

Quoted 21 lines · P J P
Hello Dan, all
+-- On Thu, 25 Oct 2018, Daniel P. Berrangé wrote --+
| On Thu, Oct 25, 2018 at 10:52:56AM +0200, Gerd Hoffmann wrote:
| > While being at it deprecate cirrus too.
| > 
| > Reason (short version): use stdvga instead.
| > Verbose version:
| >     https://www.kraxel.org/blog/2014/10/qemu-using-cirrus-considered-harmful
| 
| 
| I don't debate the points in the blog post above that stdvga is a
| better choice, but I don't think that's enough to justify deprecating
| cirrus at this point in time, because when it then gets deleted it
| will break way too many existing deployments.
| 
| We need to socialize info in that blog post above more widely and
| especially ensure that apps are not using that by default. I don't
| see it being viable to formally deprecate it in QEMU any time soon
| though given existing usage.
Quoted 8 lines
To note, IMO there are other devices/sources in QEMU which are potential candidates for deprecation, similar to adlib etc. It'll help if we could device a process to deprecate/remove such code base. Other than maintenance it invariably also becomes source of security issues.
Ex.(similar to Fedora) we could announce such candidate on qemu-devel list and after review over a period of say a month, candidate will be deprecated/expunged. (thinking aloud)
QEMU has a deprecation process:
https://qemu.weilnetz.de/doc/qemu-doc.html#Deprecated-features
Most of the stuff deprecated is CLI args / monitor commands, etc where mgmt apps just adjust the way they are calling QEMU, so end user's VMs are largely not impacted.

Quoted 5 lines

Deprecating a device type that is widely used is not desirable because that will cause breakage of existing guests. Distros are free to disable devices in their builds if they want to reduce the scope for CVEs in packages they maintain, but again they should think carefully about how many users they are going to break by doing so.

I agree with what Daniel said. Deprecating something that is in heavy use by users just because we have trouble maintaining it not going to help the QEMU project - quite the opposite.

Cole Robinson

2018-10-26 08:49 UTC #31 lore raw

Re: [Qemu-devel] [libvirt] [PATCH 3/3] cirrus: mark as deprecated

Quoted 11 lines · Daniel P. Berrangé

Quoted 5 lines · Gerd Hoffmann
While being at it deprecate cirrus too.
Reason (short version): use stdvga instead.
Verbose version:
     https://www.kraxel.org/blog/2014/10/qemu-using-cirrus-considered-harmful
Every single one of my guests is using cirrus. This wasn't an explicit choice on my part, so I believe it is being used as the default in virt-install.

virt-manager/virt-install 2.0.0 will never explicitly set cirrus, but that release is quite new. The previous release's default for x86 was roughly:

if using spice graphics:
use qxl
elif guest os is windows:
use vga
else:
use cirrus

It was definitely sub optimal. Maybe your virt-install commands were explicitly setting --graphics vnc which would trigger cirrus in that case.

Quoted 9 lines

I don't debate the points in the blog post above that stdvga is a better choice, but I don't think that's enough to justify deprecating cirrus at this point in time, because when it then gets deleted it will break way too many existing deployments.
We need to socialize info in that blog post above more widely and especially ensure that apps are not using that by default. I don't see it being viable to formally deprecate it in QEMU any time soon though given existing usage.

Daniel P. Berrangé

2018-10-26 09:43 UTC #32 lore raw

Re: [Qemu-devel] [libvirt] [PATCH 3/3] cirrus: mark as deprecated

Quoted 23 lines · Cole Robinson

Quoted 11 lines · Daniel P. Berrangé
Quoted 5 lines · Gerd Hoffmann
While being at it deprecate cirrus too.
Reason (short version): use stdvga instead.
Verbose version:
     https://www.kraxel.org/blog/2014/10/qemu-using-cirrus-considered-harmful
Every single one of my guests is using cirrus. This wasn't an explicit choice on my part, so I believe it is being used as the default in virt-install.
virt-manager/virt-install 2.0.0 will never explicitly set cirrus, but that release is quite new. The previous release's default for x86 was roughly:
if using spice graphics:
use qxl
elif guest os is windows:
use vga
else:
use cirrus

Quoted 2 lines

It was definitely sub optimal. Maybe your virt-install commands were explicitly setting --graphics vnc which would trigger cirrus in that case.

Yep, I've always tended to use vnc, since QXL has historically been a pretty buggy & unreliable device model with guests often breaking.

Regards,
Daniel

Signature · 3 lines

|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Gerd Hoffmann

author

2018-10-29 09:12 UTC #33 lore raw

Re: [Qemu-devel] [PATCH 3/3] cirrus: mark as deprecated