Re: [PATCH v2] hw/audio/sb16: Avoid assertion by restricting I/O sampling rate range

[Qiang Liu]

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
>