Re: [Qemu-devel] [PATCH 2/3] adlib: mark as insecure and deprecated.
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
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
Re: [Qemu-devel] [PATCH 2/3] adlib: mark as insecure and deprecated.
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 ? >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.
+-- 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 ? >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.
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.
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
> ---
> 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
Re: [Qemu-devel] [PATCH 2/3] adlib: mark as insecure and deprecated.
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
---
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é
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
| 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
[Qemu-devel] [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.
Signed-off-by: Gerd Hoffmann
---
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
