All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] hw/audio/sb16: Avoid assertion by restricting I/O sampling rate range
@ 2021-06-16 10:43 Philippe Mathieu-Daudé
  2021-06-16 11:58 ` Qiang Liu
  2021-06-17  9:56 ` Gerd Hoffmann
  0 siblings, 2 replies; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-06-16 10:43 UTC (permalink / raw)
  To: qemu-devel
  Cc: Laurent Vivier, Thomas Huth, Qiang Liu,
	Philippe Mathieu-Daudé,
	Alexander Bulekov, Bandan Das, Gerd Hoffmann, Stefan Hajnoczi,
	Paolo Bonzini

While the SB16 seems to work up to 48000 Hz, the "Sound Blaster Series
Hardware Programming Guide" limit the sampling range from 4000 Hz to
44100 Hz (Section 3-9, 3-10: Digitized Sound I/O Programming, tables
3-2 and 3-3).

Later, section 6-15 (DSP Commands) is more specific regarding the 41h /
42h registers (Set digitized sound output sampling rate):

  Valid sampling rates range from 5000 to 45000 Hz inclusive.

There is no comment regarding error handling if the register is filled
with an out-of-range value.  (See also section 3-28 "8-bit or 16-bit
Auto-initialize Transfer"). Assume limits are enforced in hardware.

This fixes triggering an assertion in audio_calloc():

  #1 abort
  #2 audio_bug audio/audio.c:119:9
  #3 audio_calloc audio/audio.c:154:9
  #4 audio_pcm_sw_alloc_resources_out audio/audio_template.h:116:15
  #5 audio_pcm_sw_init_out audio/audio_template.h:175:11
  #6 audio_pcm_create_voice_pair_out audio/audio_template.h:410:9
  #7 AUD_open_out audio/audio_template.h:503:14
  #8 continue_dma8 hw/audio/sb16.c:216:20
  #9 dma_cmd8 hw/audio/sb16.c:276:5
  #10 command hw/audio/sb16.c:0
  #11 dsp_write hw/audio/sb16.c:949:13
  #12 portio_write softmmu/ioport.c:205:13
  #13 memory_region_write_accessor softmmu/memory.c:491:5
  #14 access_with_adjusted_size softmmu/memory.c:552:18
  #15 memory_region_dispatch_write softmmu/memory.c:0:13
  #16 flatview_write_continue softmmu/physmem.c:2759:23
  #17 flatview_write softmmu/physmem.c:2799:14
  #18 address_space_write softmmu/physmem.c:2891:18
  #19 cpu_outw softmmu/ioport.c:70:5

[*] http://www.baudline.com/solutions/full_duplex/sb16_pci/index.html

Fixes: 85571bc7415 ("audio merge (malc)")
Buglink: https://bugs.launchpad.net/bugs/1910603
OSS-Fuzz Report: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=29174
Tested-by: Qiang Liu <cyruscyliu@gmail.com>
Reviewed-by: Qiang Liu <cyruscyliu@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
v2: Added Qiang Liu's T-b/R-b tags ¯\_(ツ)_/¯
---
 hw/audio/sb16.c              | 14 ++++++++++
 tests/qtest/fuzz-sb16-test.c | 52 ++++++++++++++++++++++++++++++++++++
 MAINTAINERS                  |  1 +
 tests/qtest/meson.build      |  1 +
 4 files changed, 68 insertions(+)
 create mode 100644 tests/qtest/fuzz-sb16-test.c

diff --git a/hw/audio/sb16.c b/hw/audio/sb16.c
index 8b207004102..5cf121fe363 100644
--- a/hw/audio/sb16.c
+++ b/hw/audio/sb16.c
@@ -115,6 +115,9 @@ struct SB16State {
     PortioList portio_list;
 };
 
+#define SAMPLE_RATE_MIN 5000
+#define SAMPLE_RATE_MAX 45000
+
 static void SB_audio_callback (void *opaque, int free);
 
 static int magic_of_irq (int irq)
@@ -241,6 +244,17 @@ static void dma_cmd8 (SB16State *s, int mask, int dma_len)
         int tmp = (256 - s->time_const);
         s->freq = (1000000 + (tmp / 2)) / tmp;
     }
+    if (s->freq < SAMPLE_RATE_MIN) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "sampling range too low: %d, increasing to %u\n",
+                      s->freq, SAMPLE_RATE_MIN);
+        s->freq = SAMPLE_RATE_MIN;
+    } else if (s->freq > SAMPLE_RATE_MAX) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "sampling range too high: %d, decreasing to %u\n",
+                      s->freq, SAMPLE_RATE_MAX);
+        s->freq = SAMPLE_RATE_MAX;
+    }
 
     if (dma_len != -1) {
         s->block_size = dma_len << s->fmt_stereo;
diff --git a/tests/qtest/fuzz-sb16-test.c b/tests/qtest/fuzz-sb16-test.c
new file mode 100644
index 00000000000..51030cd7dc4
--- /dev/null
+++ b/tests/qtest/fuzz-sb16-test.c
@@ -0,0 +1,52 @@
+/*
+ * QTest fuzzer-generated testcase for sb16 audio device
+ *
+ * Copyright (c) 2021 Philippe Mathieu-Daudé <f4bug@amsat.org>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "libqos/libqtest.h"
+
+/*
+ * This used to trigger the assert in audio_calloc
+ * https://bugs.launchpad.net/qemu/+bug/1910603
+ */
+static void test_fuzz_sb16_0x1c(void)
+{
+    QTestState *s = qtest_init("-M q35 -display none "
+                               "-device sb16,audiodev=snd0 "
+                               "-audiodev none,id=snd0");
+    qtest_outw(s, 0x22c, 0x41);
+    qtest_outb(s, 0x22c, 0x00);
+    qtest_outw(s, 0x22c, 0x1004);
+    qtest_outw(s, 0x22c, 0x001c);
+    qtest_quit(s);
+}
+
+static void test_fuzz_sb16_0x91(void)
+{
+    QTestState *s = qtest_init("-M pc -display none "
+                               "-device sb16,audiodev=none "
+                               "-audiodev id=none,driver=none");
+    qtest_outw(s, 0x22c, 0xf141);
+    qtest_outb(s, 0x22c, 0x00);
+    qtest_outb(s, 0x22c, 0x24);
+    qtest_outb(s, 0x22c, 0x91);
+    qtest_quit(s);
+}
+
+int main(int argc, char **argv)
+{
+    const char *arch = qtest_get_arch();
+
+    g_test_init(&argc, &argv, NULL);
+
+   if (strcmp(arch, "i386") == 0) {
+        qtest_add_func("fuzz/test_fuzz_sb16/1c", test_fuzz_sb16_0x1c);
+        qtest_add_func("fuzz/test_fuzz_sb16/91", test_fuzz_sb16_0x91);
+   }
+
+   return g_test_run();
+}
diff --git a/MAINTAINERS b/MAINTAINERS
index 7d9cd290426..d619ea8fcd1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2221,6 +2221,7 @@ F: qapi/audio.json
 F: tests/qtest/ac97-test.c
 F: tests/qtest/es1370-test.c
 F: tests/qtest/intel-hda-test.c
+F: tests/qtest/fuzz-sb16-test.c
 
 Block layer core
 M: Kevin Wolf <kwolf@redhat.com>
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index c3a223a83d6..b03e8541700 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -20,6 +20,7 @@
 qtests_generic = \
   (config_all_devices.has_key('CONFIG_MEGASAS_SCSI_PCI') ? ['fuzz-megasas-test'] : []) + \
   (config_all_devices.has_key('CONFIG_VIRTIO_SCSI') ? ['fuzz-virtio-scsi-test'] : []) + \
+  (config_all_devices.has_key('CONFIG_SB16') ? ['fuzz-sb16-test'] : []) + \
   [
   'cdrom-test',
   'device-introspect-test',
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] hw/audio/sb16: Avoid assertion by restricting I/O sampling rate range
  2021-06-16 10:43 [PATCH v2] hw/audio/sb16: Avoid assertion by restricting I/O sampling rate range Philippe Mathieu-Daudé
@ 2021-06-16 11:58 ` Qiang Liu
  2021-06-16 12:47   ` Philippe Mathieu-Daudé
  2021-06-17  9:56 ` Gerd Hoffmann
  1 sibling, 1 reply; 8+ messages in thread
From: Qiang Liu @ 2021-06-16 11:58 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Laurent Vivier, Thomas Huth, qemu-devel, Alexander Bulekov,
	Bandan Das, Gerd Hoffmann, Stefan Hajnoczi, Paolo Bonzini

Thx. I learned a lot about contributing to QEMU from this discussion!

Best,
Qiang

On Wed, Jun 16, 2021 at 6:43 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> While the SB16 seems to work up to 48000 Hz, the "Sound Blaster Series
> Hardware Programming Guide" limit the sampling range from 4000 Hz to
> 44100 Hz (Section 3-9, 3-10: Digitized Sound I/O Programming, tables
> 3-2 and 3-3).
>
> Later, section 6-15 (DSP Commands) is more specific regarding the 41h /
> 42h registers (Set digitized sound output sampling rate):
>
>   Valid sampling rates range from 5000 to 45000 Hz inclusive.
>
> There is no comment regarding error handling if the register is filled
> with an out-of-range value.  (See also section 3-28 "8-bit or 16-bit
> Auto-initialize Transfer"). Assume limits are enforced in hardware.
>
> This fixes triggering an assertion in audio_calloc():
>
>   #1 abort
>   #2 audio_bug audio/audio.c:119:9
>   #3 audio_calloc audio/audio.c:154:9
>   #4 audio_pcm_sw_alloc_resources_out audio/audio_template.h:116:15
>   #5 audio_pcm_sw_init_out audio/audio_template.h:175:11
>   #6 audio_pcm_create_voice_pair_out audio/audio_template.h:410:9
>   #7 AUD_open_out audio/audio_template.h:503:14
>   #8 continue_dma8 hw/audio/sb16.c:216:20
>   #9 dma_cmd8 hw/audio/sb16.c:276:5
>   #10 command hw/audio/sb16.c:0
>   #11 dsp_write hw/audio/sb16.c:949:13
>   #12 portio_write softmmu/ioport.c:205:13
>   #13 memory_region_write_accessor softmmu/memory.c:491:5
>   #14 access_with_adjusted_size softmmu/memory.c:552:18
>   #15 memory_region_dispatch_write softmmu/memory.c:0:13
>   #16 flatview_write_continue softmmu/physmem.c:2759:23
>   #17 flatview_write softmmu/physmem.c:2799:14
>   #18 address_space_write softmmu/physmem.c:2891:18
>   #19 cpu_outw softmmu/ioport.c:70:5
>
> [*] http://www.baudline.com/solutions/full_duplex/sb16_pci/index.html
>
> Fixes: 85571bc7415 ("audio merge (malc)")
> Buglink: https://bugs.launchpad.net/bugs/1910603
> OSS-Fuzz Report: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=29174
> Tested-by: Qiang Liu <cyruscyliu@gmail.com>
> Reviewed-by: Qiang Liu <cyruscyliu@gmail.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> v2: Added Qiang Liu's T-b/R-b tags ¯\_(ツ)_/¯
> ---
>  hw/audio/sb16.c              | 14 ++++++++++
>  tests/qtest/fuzz-sb16-test.c | 52 ++++++++++++++++++++++++++++++++++++
>  MAINTAINERS                  |  1 +
>  tests/qtest/meson.build      |  1 +
>  4 files changed, 68 insertions(+)
>  create mode 100644 tests/qtest/fuzz-sb16-test.c
>
> diff --git a/hw/audio/sb16.c b/hw/audio/sb16.c
> index 8b207004102..5cf121fe363 100644
> --- a/hw/audio/sb16.c
> +++ b/hw/audio/sb16.c
> @@ -115,6 +115,9 @@ struct SB16State {
>      PortioList portio_list;
>  };
>
> +#define SAMPLE_RATE_MIN 5000
> +#define SAMPLE_RATE_MAX 45000
> +
>  static void SB_audio_callback (void *opaque, int free);
>
>  static int magic_of_irq (int irq)
> @@ -241,6 +244,17 @@ static void dma_cmd8 (SB16State *s, int mask, int dma_len)
>          int tmp = (256 - s->time_const);
>          s->freq = (1000000 + (tmp / 2)) / tmp;
>      }
> +    if (s->freq < SAMPLE_RATE_MIN) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "sampling range too low: %d, increasing to %u\n",
> +                      s->freq, SAMPLE_RATE_MIN);
> +        s->freq = SAMPLE_RATE_MIN;
> +    } else if (s->freq > SAMPLE_RATE_MAX) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "sampling range too high: %d, decreasing to %u\n",
> +                      s->freq, SAMPLE_RATE_MAX);
> +        s->freq = SAMPLE_RATE_MAX;
> +    }
>
>      if (dma_len != -1) {
>          s->block_size = dma_len << s->fmt_stereo;
> diff --git a/tests/qtest/fuzz-sb16-test.c b/tests/qtest/fuzz-sb16-test.c
> new file mode 100644
> index 00000000000..51030cd7dc4
> --- /dev/null
> +++ b/tests/qtest/fuzz-sb16-test.c
> @@ -0,0 +1,52 @@
> +/*
> + * QTest fuzzer-generated testcase for sb16 audio device
> + *
> + * Copyright (c) 2021 Philippe Mathieu-Daudé <f4bug@amsat.org>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "libqos/libqtest.h"
> +
> +/*
> + * This used to trigger the assert in audio_calloc
> + * https://bugs.launchpad.net/qemu/+bug/1910603
> + */
> +static void test_fuzz_sb16_0x1c(void)
> +{
> +    QTestState *s = qtest_init("-M q35 -display none "
> +                               "-device sb16,audiodev=snd0 "
> +                               "-audiodev none,id=snd0");
> +    qtest_outw(s, 0x22c, 0x41);
> +    qtest_outb(s, 0x22c, 0x00);
> +    qtest_outw(s, 0x22c, 0x1004);
> +    qtest_outw(s, 0x22c, 0x001c);
> +    qtest_quit(s);
> +}
> +
> +static void test_fuzz_sb16_0x91(void)
> +{
> +    QTestState *s = qtest_init("-M pc -display none "
> +                               "-device sb16,audiodev=none "
> +                               "-audiodev id=none,driver=none");
> +    qtest_outw(s, 0x22c, 0xf141);
> +    qtest_outb(s, 0x22c, 0x00);
> +    qtest_outb(s, 0x22c, 0x24);
> +    qtest_outb(s, 0x22c, 0x91);
> +    qtest_quit(s);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    const char *arch = qtest_get_arch();
> +
> +    g_test_init(&argc, &argv, NULL);
> +
> +   if (strcmp(arch, "i386") == 0) {
> +        qtest_add_func("fuzz/test_fuzz_sb16/1c", test_fuzz_sb16_0x1c);
> +        qtest_add_func("fuzz/test_fuzz_sb16/91", test_fuzz_sb16_0x91);
> +   }
> +
> +   return g_test_run();
> +}
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7d9cd290426..d619ea8fcd1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2221,6 +2221,7 @@ F: qapi/audio.json
>  F: tests/qtest/ac97-test.c
>  F: tests/qtest/es1370-test.c
>  F: tests/qtest/intel-hda-test.c
> +F: tests/qtest/fuzz-sb16-test.c
>
>  Block layer core
>  M: Kevin Wolf <kwolf@redhat.com>
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index c3a223a83d6..b03e8541700 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -20,6 +20,7 @@
>  qtests_generic = \
>    (config_all_devices.has_key('CONFIG_MEGASAS_SCSI_PCI') ? ['fuzz-megasas-test'] : []) + \
>    (config_all_devices.has_key('CONFIG_VIRTIO_SCSI') ? ['fuzz-virtio-scsi-test'] : []) + \
> +  (config_all_devices.has_key('CONFIG_SB16') ? ['fuzz-sb16-test'] : []) + \
>    [
>    'cdrom-test',
>    'device-introspect-test',
> --
> 2.31.1
>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] hw/audio/sb16: Avoid assertion by restricting I/O sampling rate range
  2021-06-16 11:58 ` Qiang Liu
@ 2021-06-16 12:47   ` Philippe Mathieu-Daudé
  2021-06-17  9:55     ` Gerd Hoffmann
  0 siblings, 1 reply; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-06-16 12:47 UTC (permalink / raw)
  To: Qiang Liu
  Cc: Laurent Vivier, Thomas Huth, qemu-devel, Alexander Bulekov,
	Bandan Das, Gerd Hoffmann, Stefan Hajnoczi, Paolo Bonzini

On 6/16/21 1:58 PM, Qiang Liu wrote:
> Thx. I learned a lot about contributing to QEMU from this discussion!

I think this was a misunderstanding with Gerd, the maintainer.

Maintainers use some tools to ease their patch-by-email workflow.
As a tester/reviewer you simply reply to a patch with a "Reviewed-by"
or "Tested-by" tag (with your name and email) and the tools will
collect your tags. Then the maintainer take the patches with the
tags amended. So a v2 shouldn't be necessary normally. And this is
certainly not a task of the reviewer to resend the patch as v2 adding
its own R-b/T-b tags.
It might help you to look at the list mail archives:
https://lists.gnu.org/archive/html/qemu-devel/
or even better subscribe to the list (high traffic! you need to filter
for your interests).

> Best,
> Qiang
> 
> On Wed, Jun 16, 2021 at 6:43 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> While the SB16 seems to work up to 48000 Hz, the "Sound Blaster Series
>> Hardware Programming Guide" limit the sampling range from 4000 Hz to
>> 44100 Hz (Section 3-9, 3-10: Digitized Sound I/O Programming, tables
>> 3-2 and 3-3).
>>
>> Later, section 6-15 (DSP Commands) is more specific regarding the 41h /
>> 42h registers (Set digitized sound output sampling rate):
>>
>>   Valid sampling rates range from 5000 to 45000 Hz inclusive.
>>
>> There is no comment regarding error handling if the register is filled
>> with an out-of-range value.  (See also section 3-28 "8-bit or 16-bit
>> Auto-initialize Transfer"). Assume limits are enforced in hardware.
>>
>> This fixes triggering an assertion in audio_calloc():
>>
>>   #1 abort
>>   #2 audio_bug audio/audio.c:119:9
>>   #3 audio_calloc audio/audio.c:154:9
>>   #4 audio_pcm_sw_alloc_resources_out audio/audio_template.h:116:15
>>   #5 audio_pcm_sw_init_out audio/audio_template.h:175:11
>>   #6 audio_pcm_create_voice_pair_out audio/audio_template.h:410:9
>>   #7 AUD_open_out audio/audio_template.h:503:14
>>   #8 continue_dma8 hw/audio/sb16.c:216:20
>>   #9 dma_cmd8 hw/audio/sb16.c:276:5
>>   #10 command hw/audio/sb16.c:0
>>   #11 dsp_write hw/audio/sb16.c:949:13
>>   #12 portio_write softmmu/ioport.c:205:13
>>   #13 memory_region_write_accessor softmmu/memory.c:491:5
>>   #14 access_with_adjusted_size softmmu/memory.c:552:18
>>   #15 memory_region_dispatch_write softmmu/memory.c:0:13
>>   #16 flatview_write_continue softmmu/physmem.c:2759:23
>>   #17 flatview_write softmmu/physmem.c:2799:14
>>   #18 address_space_write softmmu/physmem.c:2891:18
>>   #19 cpu_outw softmmu/ioport.c:70:5
>>
>> [*] http://www.baudline.com/solutions/full_duplex/sb16_pci/index.html
>>
>> Fixes: 85571bc7415 ("audio merge (malc)")
>> Buglink: https://bugs.launchpad.net/bugs/1910603
>> OSS-Fuzz Report: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=29174
>> Tested-by: Qiang Liu <cyruscyliu@gmail.com>
>> Reviewed-by: Qiang Liu <cyruscyliu@gmail.com>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> v2: Added Qiang Liu's T-b/R-b tags ¯\_(ツ)_/¯
>> ---
>>  hw/audio/sb16.c              | 14 ++++++++++
>>  tests/qtest/fuzz-sb16-test.c | 52 ++++++++++++++++++++++++++++++++++++
>>  MAINTAINERS                  |  1 +
>>  tests/qtest/meson.build      |  1 +
>>  4 files changed, 68 insertions(+)
>>  create mode 100644 tests/qtest/fuzz-sb16-test.c
>>
>> diff --git a/hw/audio/sb16.c b/hw/audio/sb16.c
>> index 8b207004102..5cf121fe363 100644
>> --- a/hw/audio/sb16.c
>> +++ b/hw/audio/sb16.c
>> @@ -115,6 +115,9 @@ struct SB16State {
>>      PortioList portio_list;
>>  };
>>
>> +#define SAMPLE_RATE_MIN 5000
>> +#define SAMPLE_RATE_MAX 45000
>> +
>>  static void SB_audio_callback (void *opaque, int free);
>>
>>  static int magic_of_irq (int irq)
>> @@ -241,6 +244,17 @@ static void dma_cmd8 (SB16State *s, int mask, int dma_len)
>>          int tmp = (256 - s->time_const);
>>          s->freq = (1000000 + (tmp / 2)) / tmp;
>>      }
>> +    if (s->freq < SAMPLE_RATE_MIN) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "sampling range too low: %d, increasing to %u\n",
>> +                      s->freq, SAMPLE_RATE_MIN);
>> +        s->freq = SAMPLE_RATE_MIN;
>> +    } else if (s->freq > SAMPLE_RATE_MAX) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "sampling range too high: %d, decreasing to %u\n",
>> +                      s->freq, SAMPLE_RATE_MAX);
>> +        s->freq = SAMPLE_RATE_MAX;
>> +    }
>>
>>      if (dma_len != -1) {
>>          s->block_size = dma_len << s->fmt_stereo;
>> diff --git a/tests/qtest/fuzz-sb16-test.c b/tests/qtest/fuzz-sb16-test.c
>> new file mode 100644
>> index 00000000000..51030cd7dc4
>> --- /dev/null
>> +++ b/tests/qtest/fuzz-sb16-test.c
>> @@ -0,0 +1,52 @@
>> +/*
>> + * QTest fuzzer-generated testcase for sb16 audio device
>> + *
>> + * Copyright (c) 2021 Philippe Mathieu-Daudé <f4bug@amsat.org>
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "libqos/libqtest.h"
>> +
>> +/*
>> + * This used to trigger the assert in audio_calloc
>> + * https://bugs.launchpad.net/qemu/+bug/1910603
>> + */
>> +static void test_fuzz_sb16_0x1c(void)
>> +{
>> +    QTestState *s = qtest_init("-M q35 -display none "
>> +                               "-device sb16,audiodev=snd0 "
>> +                               "-audiodev none,id=snd0");
>> +    qtest_outw(s, 0x22c, 0x41);
>> +    qtest_outb(s, 0x22c, 0x00);
>> +    qtest_outw(s, 0x22c, 0x1004);
>> +    qtest_outw(s, 0x22c, 0x001c);
>> +    qtest_quit(s);
>> +}
>> +
>> +static void test_fuzz_sb16_0x91(void)
>> +{
>> +    QTestState *s = qtest_init("-M pc -display none "
>> +                               "-device sb16,audiodev=none "
>> +                               "-audiodev id=none,driver=none");
>> +    qtest_outw(s, 0x22c, 0xf141);
>> +    qtest_outb(s, 0x22c, 0x00);
>> +    qtest_outb(s, 0x22c, 0x24);
>> +    qtest_outb(s, 0x22c, 0x91);
>> +    qtest_quit(s);
>> +}
>> +
>> +int main(int argc, char **argv)
>> +{
>> +    const char *arch = qtest_get_arch();
>> +
>> +    g_test_init(&argc, &argv, NULL);
>> +
>> +   if (strcmp(arch, "i386") == 0) {
>> +        qtest_add_func("fuzz/test_fuzz_sb16/1c", test_fuzz_sb16_0x1c);
>> +        qtest_add_func("fuzz/test_fuzz_sb16/91", test_fuzz_sb16_0x91);
>> +   }
>> +
>> +   return g_test_run();
>> +}
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 7d9cd290426..d619ea8fcd1 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -2221,6 +2221,7 @@ F: qapi/audio.json
>>  F: tests/qtest/ac97-test.c
>>  F: tests/qtest/es1370-test.c
>>  F: tests/qtest/intel-hda-test.c
>> +F: tests/qtest/fuzz-sb16-test.c
>>
>>  Block layer core
>>  M: Kevin Wolf <kwolf@redhat.com>
>> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
>> index c3a223a83d6..b03e8541700 100644
>> --- a/tests/qtest/meson.build
>> +++ b/tests/qtest/meson.build
>> @@ -20,6 +20,7 @@
>>  qtests_generic = \
>>    (config_all_devices.has_key('CONFIG_MEGASAS_SCSI_PCI') ? ['fuzz-megasas-test'] : []) + \
>>    (config_all_devices.has_key('CONFIG_VIRTIO_SCSI') ? ['fuzz-virtio-scsi-test'] : []) + \
>> +  (config_all_devices.has_key('CONFIG_SB16') ? ['fuzz-sb16-test'] : []) + \
>>    [
>>    'cdrom-test',
>>    'device-introspect-test',
>> --
>> 2.31.1
>>
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] hw/audio/sb16: Avoid assertion by restricting I/O sampling rate range
  2021-06-16 12:47   ` Philippe Mathieu-Daudé
@ 2021-06-17  9:55     ` Gerd Hoffmann
  0 siblings, 0 replies; 8+ messages in thread
From: Gerd Hoffmann @ 2021-06-17  9:55 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Laurent Vivier, Thomas Huth, Qiang Liu, qemu-devel,
	Alexander Bulekov, Bandan Das, Stefan Hajnoczi, Paolo Bonzini

On Wed, Jun 16, 2021 at 02:47:35PM +0200, Philippe Mathieu-Daudé wrote:
> On 6/16/21 1:58 PM, Qiang Liu wrote:
> > Thx. I learned a lot about contributing to QEMU from this discussion!
> 
> I think this was a misunderstanding with Gerd, the maintainer.

Indeed.

> Maintainers use some tools to ease their patch-by-email workflow.
> As a tester/reviewer you simply reply to a patch with a "Reviewed-by"
> or "Tested-by" tag (with your name and email) and the tools will
> collect your tags. Then the maintainer take the patches with the
> tags amended. So a v2 shouldn't be necessary normally.

Correct (I'm using https://pypi.org/project/b4/ btw).

I didn't follow the mail thread that closely and had the false
impression this discussion was about other tags (b4 wouldn't
create Fixes: tags for you ...).

Sorry for the confusion.

take care,
  Gerd



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] hw/audio/sb16: Avoid assertion by restricting I/O sampling rate range
  2021-06-16 10:43 [PATCH v2] hw/audio/sb16: Avoid assertion by restricting I/O sampling rate range Philippe Mathieu-Daudé
  2021-06-16 11:58 ` Qiang Liu
@ 2021-06-17  9:56 ` Gerd Hoffmann
  2021-06-22  8:54   ` Qiang Liu
  1 sibling, 1 reply; 8+ messages in thread
From: Gerd Hoffmann @ 2021-06-17  9:56 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Laurent Vivier, Thomas Huth, Qiang Liu, qemu-devel,
	Alexander Bulekov, Bandan Das, Stefan Hajnoczi, Paolo Bonzini

On Wed, Jun 16, 2021 at 12:43:49PM +0200, Philippe Mathieu-Daudé wrote:
> While the SB16 seems to work up to 48000 Hz, the "Sound Blaster Series
> Hardware Programming Guide" limit the sampling range from 4000 Hz to
> 44100 Hz (Section 3-9, 3-10: Digitized Sound I/O Programming, tables
> 3-2 and 3-3).
> 
> Later, section 6-15 (DSP Commands) is more specific regarding the 41h /
> 42h registers (Set digitized sound output sampling rate):
> 
>   Valid sampling rates range from 5000 to 45000 Hz inclusive.
> 
> There is no comment regarding error handling if the register is filled
> with an out-of-range value.  (See also section 3-28 "8-bit or 16-bit
> Auto-initialize Transfer"). Assume limits are enforced in hardware.
> 
> This fixes triggering an assertion in audio_calloc():
> 
>   #1 abort
>   #2 audio_bug audio/audio.c:119:9
>   #3 audio_calloc audio/audio.c:154:9
>   #4 audio_pcm_sw_alloc_resources_out audio/audio_template.h:116:15
>   #5 audio_pcm_sw_init_out audio/audio_template.h:175:11
>   #6 audio_pcm_create_voice_pair_out audio/audio_template.h:410:9
>   #7 AUD_open_out audio/audio_template.h:503:14
>   #8 continue_dma8 hw/audio/sb16.c:216:20
>   #9 dma_cmd8 hw/audio/sb16.c:276:5
>   #10 command hw/audio/sb16.c:0
>   #11 dsp_write hw/audio/sb16.c:949:13
>   #12 portio_write softmmu/ioport.c:205:13
>   #13 memory_region_write_accessor softmmu/memory.c:491:5
>   #14 access_with_adjusted_size softmmu/memory.c:552:18
>   #15 memory_region_dispatch_write softmmu/memory.c:0:13
>   #16 flatview_write_continue softmmu/physmem.c:2759:23
>   #17 flatview_write softmmu/physmem.c:2799:14
>   #18 address_space_write softmmu/physmem.c:2891:18
>   #19 cpu_outw softmmu/ioport.c:70:5
> 
> [*] http://www.baudline.com/solutions/full_duplex/sb16_pci/index.html
> 
> Fixes: 85571bc7415 ("audio merge (malc)")
> Buglink: https://bugs.launchpad.net/bugs/1910603
> OSS-Fuzz Report: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=29174
> Tested-by: Qiang Liu <cyruscyliu@gmail.com>
> Reviewed-by: Qiang Liu <cyruscyliu@gmail.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Added to audio queue.

thanks,
  Gerd



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] hw/audio/sb16: Avoid assertion by restricting I/O sampling rate range
  2021-06-17  9:56 ` Gerd Hoffmann
@ 2021-06-22  8:54   ` Qiang Liu
  2021-06-22  9:15     ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 8+ messages in thread
From: Qiang Liu @ 2021-06-22  8:54 UTC (permalink / raw)
  To: Gerd Hoffmann
  Cc: Laurent Vivier, Thomas Huth, Philippe Mathieu-Daudé,
	qemu-devel, Alexander Bulekov, Bandan Das, Stefan Hajnoczi,
	Paolo Bonzini

Hi folks,

With this patch, having tested more, I find another way to trigger the
assertion.
I found it just now such that I did a quick investigation and reported
it to you. I
hope this would prevent merging this patch.

Brief analysis:
This existing patch limits s->freq in dma_cmd8 before continue_dma8 followed
by AUD_open_out. It's good to prevent the flow by this path. However, we can
directly call continue_dma8 via command 0xd4 but there is no limit check there.

To trigger this assertion, we can manipulate s->freq in the following way.
1. dsp_write, offset=0xc, val=0x41
Because s->needed_bytes = 0, command() is called.
```
case 0x41:
    s->freq = -1;
    s->time_const = -1;
    s->needed_bytes = 2; // look at here
...
s->cmd = cmd; // 0x41, and here
```

2. dsp_write, offset=0xc, val=0x14
Because s->needed_bytes = 2, complete() is called.
```
s->in2_data[s->in_index++] = 0x14; // remembere this
s->needed_bytes = 0
```
Because s->cmd = 0x41, s->freq will be reset.
```
case 0x41:
case 0x42:
    s->freq = dsp_get_hilo (s);
                    // return s->in2_data[--s->in_index]
                    // s->freq will be 0x14, there is no check ...
```

3. dsp_write, offset=0xc, val=0xd4
Call continue_dma8 directly then we can trigger this assertion because
s->freq is too small.

Maybe we can fix this flaw by adding s->freq check after s->freq =
dsp_get_hilo (s) in the second step?

Best,
Qiang

On Thu, Jun 17, 2021 at 5:56 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Wed, Jun 16, 2021 at 12:43:49PM +0200, Philippe Mathieu-Daudé wrote:
> > While the SB16 seems to work up to 48000 Hz, the "Sound Blaster Series
> > Hardware Programming Guide" limit the sampling range from 4000 Hz to
> > 44100 Hz (Section 3-9, 3-10: Digitized Sound I/O Programming, tables
> > 3-2 and 3-3).
> >
> > Later, section 6-15 (DSP Commands) is more specific regarding the 41h /
> > 42h registers (Set digitized sound output sampling rate):
> >
> >   Valid sampling rates range from 5000 to 45000 Hz inclusive.
> >
> > There is no comment regarding error handling if the register is filled
> > with an out-of-range value.  (See also section 3-28 "8-bit or 16-bit
> > Auto-initialize Transfer"). Assume limits are enforced in hardware.
> >
> > This fixes triggering an assertion in audio_calloc():
> >
> >   #1 abort
> >   #2 audio_bug audio/audio.c:119:9
> >   #3 audio_calloc audio/audio.c:154:9
> >   #4 audio_pcm_sw_alloc_resources_out audio/audio_template.h:116:15
> >   #5 audio_pcm_sw_init_out audio/audio_template.h:175:11
> >   #6 audio_pcm_create_voice_pair_out audio/audio_template.h:410:9
> >   #7 AUD_open_out audio/audio_template.h:503:14
> >   #8 continue_dma8 hw/audio/sb16.c:216:20
> >   #9 dma_cmd8 hw/audio/sb16.c:276:5
> >   #10 command hw/audio/sb16.c:0
> >   #11 dsp_write hw/audio/sb16.c:949:13
> >   #12 portio_write softmmu/ioport.c:205:13
> >   #13 memory_region_write_accessor softmmu/memory.c:491:5
> >   #14 access_with_adjusted_size softmmu/memory.c:552:18
> >   #15 memory_region_dispatch_write softmmu/memory.c:0:13
> >   #16 flatview_write_continue softmmu/physmem.c:2759:23
> >   #17 flatview_write softmmu/physmem.c:2799:14
> >   #18 address_space_write softmmu/physmem.c:2891:18
> >   #19 cpu_outw softmmu/ioport.c:70:5
> >
> > [*] http://www.baudline.com/solutions/full_duplex/sb16_pci/index.html
> >
> > Fixes: 85571bc7415 ("audio merge (malc)")
> > Buglink: https://bugs.launchpad.net/bugs/1910603
> > OSS-Fuzz Report: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=29174
> > Tested-by: Qiang Liu <cyruscyliu@gmail.com>
> > Reviewed-by: Qiang Liu <cyruscyliu@gmail.com>
> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> Added to audio queue.
>
> thanks,
>   Gerd
>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] hw/audio/sb16: Avoid assertion by restricting I/O sampling rate range
  2021-06-22  8:54   ` Qiang Liu
@ 2021-06-22  9:15     ` Philippe Mathieu-Daudé
  2021-06-22 10:13       ` Qiang Liu
  0 siblings, 1 reply; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-06-22  9:15 UTC (permalink / raw)
  To: Qiang Liu, Gerd Hoffmann
  Cc: Laurent Vivier, Thomas Huth, qemu-devel, Alexander Bulekov,
	Bandan Das, Stefan Hajnoczi, Paolo Bonzini

On 6/22/21 10:54 AM, Qiang Liu wrote:
> Hi folks,
> 
> With this patch, having tested more, I find another way to trigger the
> assertion.
> I found it just now such that I did a quick investigation and reported
> it to you. I
> hope this would prevent merging this patch.

No need to prevent merging this patch as it already fixes problems.

> Brief analysis:
> This existing patch limits s->freq in dma_cmd8 before continue_dma8 followed
> by AUD_open_out. It's good to prevent the flow by this path. However, we can
> directly call continue_dma8 via command 0xd4 but there is no limit check there.
> 
> To trigger this assertion, we can manipulate s->freq in the following way.
> 1. dsp_write, offset=0xc, val=0x41
> Because s->needed_bytes = 0, command() is called.
> ```
> case 0x41:
>     s->freq = -1;
>     s->time_const = -1;
>     s->needed_bytes = 2; // look at here
> ...
> s->cmd = cmd; // 0x41, and here
> ```
> 
> 2. dsp_write, offset=0xc, val=0x14
> Because s->needed_bytes = 2, complete() is called.
> ```
> s->in2_data[s->in_index++] = 0x14; // remembere this
> s->needed_bytes = 0
> ```
> Because s->cmd = 0x41, s->freq will be reset.
> ```
> case 0x41:
> case 0x42:
>     s->freq = dsp_get_hilo (s);
>                     // return s->in2_data[--s->in_index]
>                     // s->freq will be 0x14, there is no check ...
> ```
> 
> 3. dsp_write, offset=0xc, val=0xd4
> Call continue_dma8 directly then we can trigger this assertion because
> s->freq is too small.
> 
> Maybe we can fix this flaw by adding s->freq check after s->freq =
> dsp_get_hilo (s) in the second step?

Do you mind sending a new patch with reproducer and your fix?

> Best,
> Qiang


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] hw/audio/sb16: Avoid assertion by restricting I/O sampling rate range
  2021-06-22  9:15     ` Philippe Mathieu-Daudé
@ 2021-06-22 10:13       ` Qiang Liu
  0 siblings, 0 replies; 8+ messages in thread
From: Qiang Liu @ 2021-06-22 10:13 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Laurent Vivier, Thomas Huth, qemu-devel, Alexander Bulekov,
	Bandan Das, Gerd Hoffmann, Stefan Hajnoczi, Paolo Bonzini

On Tue, Jun 22, 2021 at 5:16 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 6/22/21 10:54 AM, Qiang Liu wrote:
> > Hi folks,
> >
> > With this patch, having tested more, I find another way to trigger the
> > assertion.
> > I found it just now such that I did a quick investigation and reported
> > it to you. I
> > hope this would prevent merging this patch.
>
> No need to prevent merging this patch as it already fixes problems.
OK. I see.

> > Brief analysis:
> > This existing patch limits s->freq in dma_cmd8 before continue_dma8 followed
> > by AUD_open_out. It's good to prevent the flow by this path. However, we can
> > directly call continue_dma8 via command 0xd4 but there is no limit check there.
> >
> > To trigger this assertion, we can manipulate s->freq in the following way.
> > 1. dsp_write, offset=0xc, val=0x41
> > Because s->needed_bytes = 0, command() is called.
> > ```
> > case 0x41:
> >     s->freq = -1;
> >     s->time_const = -1;
> >     s->needed_bytes = 2; // look at here
> > ...
> > s->cmd = cmd; // 0x41, and here
> > ```
> >
> > 2. dsp_write, offset=0xc, val=0x14
> > Because s->needed_bytes = 2, complete() is called.
> > ```
> > s->in2_data[s->in_index++] = 0x14; // remembere this
> > s->needed_bytes = 0
> > ```
> > Because s->cmd = 0x41, s->freq will be reset.
> > ```
> > case 0x41:
> > case 0x42:
> >     s->freq = dsp_get_hilo (s);
> >                     // return s->in2_data[--s->in_index]
> >                     // s->freq will be 0x14, there is no check ...
> > ```
> >
> > 3. dsp_write, offset=0xc, val=0xd4
> > Call continue_dma8 directly then we can trigger this assertion because
> > s->freq is too small.
> >
> > Maybe we can fix this flaw by adding s->freq check after s->freq =
> > dsp_get_hilo (s) in the second step?
>
> Do you mind sending a new patch with reproducer and your fix?
Sure, no problem.

> > Best,
> > Qiang


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-06-22 10:14 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-16 10:43 [PATCH v2] hw/audio/sb16: Avoid assertion by restricting I/O sampling rate range Philippe Mathieu-Daudé
2021-06-16 11:58 ` Qiang Liu
2021-06-16 12:47   ` Philippe Mathieu-Daudé
2021-06-17  9:55     ` Gerd Hoffmann
2021-06-17  9:56 ` Gerd Hoffmann
2021-06-22  8:54   ` Qiang Liu
2021-06-22  9:15     ` Philippe Mathieu-Daudé
2021-06-22 10:13       ` Qiang Liu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.