1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH 0/3] RfC: add support for deprecated devices.
  4. View patch

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

Gerd Hoffmann posted 3 patches 7 years ago
[Qemu-devel] [PATCH 2/3] adlib: mark as insecure and deprecated.
Posted by Gerd Hoffmann 7 years ago 
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 <kraxel@redhat.com>
---
 hw/audio/adlib.c     | 1 +
 qemu-deprecated.texi | 4 ++++
 2 files changed, 5 insertions(+)

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)
-- 
2.9.3
Re: [libvirt] [PATCH 2/3] adlib: mark as insecure and deprecated.
Posted by P J P 7 years ago 
+-- 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 <wangjunqing@huawei.com>


| 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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [Qemu-devel] [libvirt] [PATCH 2/3] adlib: mark as insecure and deprecated.
Posted by Daniel P. Berrangé 7 years ago 
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 <wangjunqing@huawei.com>

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.

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

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

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

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 :|
Re: [Qemu-devel] [libvirt] [PATCH 2/3] adlib: mark as insecure and deprecated.
Posted by P J P 7 years ago 
+-- 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 <wangjunqing@huawei.com>
| 
| 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
Re: [Qemu-devel] [libvirt] [PATCH 2/3] adlib: mark as insecure and deprecated.
Posted by Daniel P. Berrangé 7 years ago 
On Fri, Oct 26, 2018 at 12:38:53PM +0530, P J P wrote:
> +-- 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 <wangjunqing@huawei.com>
> | 
> | 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.

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
-- 
|: 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 :|
Re: [Qemu-devel] [libvirt] [PATCH 2/3] adlib: mark as insecure and deprecated.
Posted by P J P 7 years ago 
+-- 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
Re: [libvirt] [Qemu-devel] [PATCH 2/3] adlib: mark as insecure and deprecated.
Posted by Philippe Mathieu-Daudé 7 years ago 
On 25/10/18 10:52, 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])
> 
> 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"

> users and schedule it for removal according to our deprecation policy.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>   hw/audio/adlib.c     | 1 +
>   qemu-deprecated.texi | 4 ++++
>   2 files changed, 5 insertions(+)
> 
> 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)
> 

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH 2/3] adlib: mark as insecure and deprecated.
Posted by Thomas Huth 7 years ago 
On 2018-10-25 09:52, 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])
> 
> 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 <kraxel@redhat.com>
> ---
>  hw/audio/adlib.c     | 1 +
>  qemu-deprecated.texi | 4 ++++
>  2 files changed, 5 insertions(+)
> 
> 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)
> 

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 <thuth@redhat.com>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [Qemu-devel] [PATCH 2/3] adlib: mark as insecure and deprecated.
Posted by Paolo Bonzini 7 years ago 
On 25/10/2018 10:52, 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])

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
Re: [Qemu-devel] [PATCH 2/3] adlib: mark as insecure and deprecated.
Posted by P J P 7 years ago 
+-- 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
Re: [Qemu-devel] [PATCH 2/3] adlib: mark as insecure and deprecated.
Posted by Paolo Bonzini 7 years ago 
On 26/10/2018 11:34, P J P wrote:
> +-- 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];

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/).

Thanks,

Paolo
Re: [Qemu-devel] [PATCH 2/3] adlib: mark as insecure and deprecated.
Posted by P J P 7 years ago 
+-- 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
Re: [Qemu-devel] [PATCH 2/3] adlib: mark as insecure and deprecated.
Posted by Gerd Hoffmann 7 years ago 
On Fri, Oct 26, 2018 at 05:23:37PM +0530, P J P wrote:
> +-- 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.

I think we can actually do both.

cheers,
  Gerd

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.