Re: [PATCH] hw/audio/c97: fix abort in audio_calloc()

[Volker Rümelin]

Am 25.12.22 um 13:13 schrieb Qiang Liu:

Hi Qiang,

I didn't receive your email probably because the reverse DNS entry of 
your mail server isn't setup correctly.
This is from the mail header of the qemu-devel mailing list server.
X-Host-Lookup-Failed: Reverse DNS lookup failed for 220.184.252.86 (failed)

Did you see my patches at 
https://lists.nongnu.org/archive/html/qemu-devel/2022-12/msg02895.html ?
Patches 01/11 and 02/11 prevent the disturbing error message from 
audio_calloc and later patches remove the audio_calloc function.

I think the subject of your patch isn't correct. Your patch doesn't fix 
an abort in audio_calloc. In 
https://gitlab.com/qemu-project/qemu/-/issues/1393 you correctly notice 
this was already fixed.

> Section 5.10.2 of the AC97 specification (https://hands.com/~lkcl/ac97_r23.pdf)
> shows the feasibility to support for rates other than 48kHZ. Specifically,
> AC97_PCM_Front_DAC_Rate (reg 2Ch) should be from 8kHZ to 48kHZ.

I think you misread section 5.10.2 of the AC97 Revision 2.3 
specification. Section 5.10 is about S/PDIF concurrency. It doesn't say 
anything about the lowest sample rate limit without concurrent S/PDIF 
transmission. The emulated SigmaTel STAC9700 codec doesn't even have a 
S/PDIF output. But I have an example for sample rates lower than 8kHz. 
The Texas Instruments LM4546B is an AC97 codec which supports sample 
rates from 4kHz - 48kHz.

The STAC9700 is a 48kHz fixed rate codec. You won't find anything about 
the highest or lowest sample rate in its data sheet. Someone added the 
VRA and VRM modes in QEMU and the guest drivers don't seem to mind.

I would like to keep the ability to select a 1Hz sample rate, as there 
is no reason to artificially limit the lowest supported sample rate. See 
https://lists.nongnu.org/archive/html/qemu-devel/2022-10/msg03987.html

I would support a patch to limit the VRA and VRM modes to the highest 
supported rate of 48kHz. This is a technical limit for single data rate.

> Before Volker Rümelin fixed it in 12f4abf6a245 and 0cbc8bd4694f, an adversary
> could leverage this to crash QEMU.
>
> Fixes: e5c9a13e2670 ("PCI AC97 emulation by malc.")
> Reported-by: Volker Rümelin<vr_qemu@t-online.de>
> Reported-by: Qiang Liu<cyruscyliu@gmail.com>
> Resolves:https://gitlab.com/qemu-project/qemu/-/issues/1393
> Signed-off-by: Qiang Liu<cyruscyliu@gmail.com>
> ---
>   hw/audio/ac97.c | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/hw/audio/ac97.c b/hw/audio/ac97.c
> index be2dd701a4..826411e462 100644
> --- a/hw/audio/ac97.c
> +++ b/hw/audio/ac97.c
> @@ -625,9 +625,14 @@ static void nam_writew(void *opaque, uint32_t addr, 
> uint32_t val)
>           break;
>       case AC97_PCM_Front_DAC_Rate:
>           if (mixer_load(s, AC97_Extended_Audio_Ctrl_Stat) & EACS_VRA) {
> -            mixer_store(s, addr, val);
> -            dolog("Set front DAC rate to %d\n", val);
> -            open_voice(s, PO_INDEX, val);
> +            if (val >= 8000 && val <= 48000) {
> +                mixer_store(s, addr, val);
> +                dolog("Set front DAC rate to %d\n", val);
> +                open_voice(s, PO_INDEX, val);
> +            } else {
> +                dolog("Attempt to set front DAC rate to %d, but valid is"
> +                      "8-48kHZ\n", val);

This is not correct. If you limit the sample rate, you should echo back 
the closest supported sample rate. See AC'97 2.3 Section 5.8.3. It's not 
a guest error if the guest writes an unsupported sample rate to the 
Audio Sample Rate Control Registers, which means it's also not necessary 
to log this.

With best regards,
Volker

> +            }
>           } else {
>               dolog("Attempt to set front DAC rate to %d, but VRA is not set\n",
>                     val);