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

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
>

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

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

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

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
>

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

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