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

[Philippe Mathieu-Daudé]

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