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

[Qiang Liu]

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