diff mbox series

[3/3] cirrus: mark as deprecated

Message ID 20181025085256.20522-4-kraxel@redhat.com
State New
Headers show
Series RfC: add support for deprecated devices. | expand

Commit Message

Gerd Hoffmann Oct. 25, 2018, 8:52 a.m. UTC
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 <kraxel@redhat.com>
---
 hw/display/cirrus_vga.c     | 2 ++
 hw/display/cirrus_vga_isa.c | 2 ++
 qemu-deprecated.texi        | 4 ++++
 3 files changed, 8 insertions(+)

Comments

Prasad Pandit Oct. 25, 2018, 10:59 a.m. UTC | #1
+-- 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 <kraxel@redhat.com>
| ---
|  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 <pjp@fedoraproject.org>

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
Philippe Mathieu-Daudé Oct. 25, 2018, 11:17 a.m. UTC | #2
On 25/10/18 10:52, 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 <kraxel@redhat.com>
> ---
>   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";

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

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>   }
>   
>   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)
>
Thomas Huth Oct. 25, 2018, 8:28 p.m. UTC | #3
On 2018-10-25 09:52, 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 <kraxel@redhat.com>
> ---
>  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: Thomas Huth <thuth@redhat.com>
Daniel P. Berrangé Oct. 25, 2018, 8:37 p.m. UTC | #4
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

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.

> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  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)
> -- 
> 2.9.3
> 
> 

Regards,
Daniel
Prasad Pandit Oct. 26, 2018, 7:03 a.m. UTC | #5
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
Cole Robinson Oct. 26, 2018, 8:48 a.m. UTC | #6
On 10/25/2018 09:37 PM, 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
> 
> 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.

> 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é Oct. 26, 2018, 9:42 a.m. UTC | #7
On Fri, Oct 26, 2018 at 12:33:55PM +0530, P J P wrote:
>   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)

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.

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
Daniel P. Berrangé Oct. 26, 2018, 9:43 a.m. UTC | #8
On Fri, Oct 26, 2018 at 09:48:35AM +0100, Cole Robinson wrote:
> On 10/25/2018 09:37 PM, 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
> > 
> > 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.

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
Daniel P. Berrangé Oct. 26, 2018, 9:59 a.m. UTC | #9
On Fri, Oct 26, 2018 at 10:42:08AM +0100, Daniel P. Berrangé wrote:
> On Fri, Oct 26, 2018 at 12:33:55PM +0530, P J P wrote:
> >   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)
> 
> 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.
> 
> 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
Paolo Bonzini Oct. 26, 2018, 10:03 a.m. UTC | #10
On 26/10/2018 11:59, Daniel P. Berrangé wrote:
> 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
Dr. David Alan Gilbert Oct. 26, 2018, 10:40 a.m. UTC | #11
* Daniel P. Berrangé (berrange@redhat.com) wrote:
> On Fri, Oct 26, 2018 at 12:33:55PM +0530, P J P wrote:
> >   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)
> 
> 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.
> 
> 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

> Regards,
> Daniel
> -- 
> |: 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 / dgilbert@redhat.com / Manchester, UK
Christian Borntraeger Oct. 26, 2018, 1:25 p.m. UTC | #12
On 10/26/2018 11:42 AM, Daniel P. Berrangé wrote:
> On Fri, Oct 26, 2018 at 12:33:55PM +0530, P J P wrote:
>>   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)
> 
> 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.
> 
> 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.
Daniel P. Berrangé Oct. 26, 2018, 2:14 p.m. UTC | #13
On Fri, Oct 26, 2018 at 12:03:35PM +0200, Paolo Bonzini wrote:
> On 26/10/2018 11:59, Daniel P. Berrangé wrote:
> > 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...

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
Prasad Pandit Oct. 26, 2018, 6:56 p.m. UTC | #14
+-- 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
Gerd Hoffmann Oct. 29, 2018, 9:12 a.m. UTC | #15
On Thu, Oct 25, 2018 at 09:37:58PM +0100, 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
> 
> 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.

Well, getting that message to the users is the point of deprecating it.
I think in this specific case we'll need a longer (maybe much longer)
deprecation period than only two releases.  But I think it still makes
sense to deprecate it now.

In qemu it isn't the default any more since release 2.2.
libvirt switched the default not too long after that.
Not fully sure how things look like higher up the stack.

cheers,
  Gerd
Daniel P. Berrangé Oct. 30, 2018, 6 p.m. UTC | #16
On Mon, Oct 29, 2018 at 10:12:44AM +0100, Gerd Hoffmann wrote:
> On Thu, Oct 25, 2018 at 09:37:58PM +0100, 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
> > 
> > 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.
> 
> Well, getting that message to the users is the point of deprecating it.
> I think in this specific case we'll need a longer (maybe much longer)
> deprecation period than only two releases.  But I think it still makes
> sense to deprecate it now.

As mentioned elsewhere in this thread, I think it is important that
QEMU distinction between devices that are best practice / recommended
for use with KVM virt, vs devices that exist solely for sake of
emulating hardware for use with old platforms.

I agree that cirrus doesn't have a place in a KVM guest for future,
but I don't thjink that means it should be deprecated & removed from
QEMU entirely, as its still relevant from POV of QEMU's goal to
emulate old hardware platforms.


Regards,
Daniel
diff mbox series

Patch

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)