* Re: [PATCH] hw/audio/c97: fix abort in audio_calloc()
2022-12-25 12:13 [PATCH] hw/audio/c97: fix abort in audio_calloc() Qiang Liu
@ 2022-12-25 13:58 ` Christian Schoenebeck
2022-12-25 14:58 ` Bernhard Beschow
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Christian Schoenebeck @ 2022-12-25 13:58 UTC (permalink / raw)
To: qemu-devel; +Cc: Volker Rümelin, Gerd Hoffmann, Qiang Liu
On Sunday, December 25, 2022 1:13:57 PM CET Qiang Liu wrote:
> 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.
>
> 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);
> + }
Missing space between "is" and "8-48kHz" and it is "Hz" (lower z). Except of that:
Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> } else {
> dolog("Attempt to set front DAC rate to %d, but VRA is not set\n",
> val);
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] hw/audio/c97: fix abort in audio_calloc()
2022-12-25 12:13 [PATCH] hw/audio/c97: fix abort in audio_calloc() Qiang Liu
2022-12-25 13:58 ` Christian Schoenebeck
@ 2022-12-25 14:58 ` Bernhard Beschow
2022-12-25 23:16 ` Bernhard Beschow
2022-12-26 18:50 ` Volker Rümelin
3 siblings, 0 replies; 6+ messages in thread
From: Bernhard Beschow @ 2022-12-25 14:58 UTC (permalink / raw)
To: qemu-devel, Qiang Liu; +Cc: Volker Rümelin, Gerd Hoffmann
Am 25. Dezember 2022 12:13:57 UTC schrieb Qiang Liu <cyruscyliu@gmail.com>:
>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.
>
>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);
We probably want to log a guest error here. Not only does this communicate clearly where the problem is (-> in guest code) but it is also incredibly useful for development (think of reusing this code in other device models such as vt82c686b if applicable).
Best regards,
Bernhard
>+ }
> } else {
> dolog("Attempt to set front DAC rate to %d, but VRA is not set\n",
> val);
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] hw/audio/c97: fix abort in audio_calloc()
2022-12-25 12:13 [PATCH] hw/audio/c97: fix abort in audio_calloc() Qiang Liu
2022-12-25 13:58 ` Christian Schoenebeck
2022-12-25 14:58 ` Bernhard Beschow
@ 2022-12-25 23:16 ` Bernhard Beschow
2022-12-26 18:50 ` Volker Rümelin
3 siblings, 0 replies; 6+ messages in thread
From: Bernhard Beschow @ 2022-12-25 23:16 UTC (permalink / raw)
To: qemu-devel, Qiang Liu; +Cc: Volker Rümelin, Gerd Hoffmann
Am 25. Dezember 2022 12:13:57 UTC schrieb Qiang Liu <cyruscyliu@gmail.com>:
There is an 'a' missing in the topic: It should be ac97, not c97.
Best regards,
Bernhard
>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.
>
>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);
>+ }
> } else {
> dolog("Attempt to set front DAC rate to %d, but VRA is not set\n",
> val);
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] hw/audio/c97: fix abort in audio_calloc()
2022-12-25 12:13 [PATCH] hw/audio/c97: fix abort in audio_calloc() Qiang Liu
` (2 preceding siblings ...)
2022-12-25 23:16 ` Bernhard Beschow
@ 2022-12-26 18:50 ` Volker Rümelin
2022-12-28 8:15 ` Qiang Liu
3 siblings, 1 reply; 6+ messages in thread
From: Volker Rümelin @ 2022-12-26 18:50 UTC (permalink / raw)
To: Qiang Liu
Cc: Gerd Hoffmann, Christian Schoenebeck, Bernhard Beschow,
qemu-devel
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);
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] hw/audio/c97: fix abort in audio_calloc()
2022-12-26 18:50 ` Volker Rümelin
@ 2022-12-28 8:15 ` Qiang Liu
0 siblings, 0 replies; 6+ messages in thread
From: Qiang Liu @ 2022-12-28 8:15 UTC (permalink / raw)
To: Volker Rümelin
Cc: Gerd Hoffmann, Christian Schoenebeck, Bernhard Beschow,
qemu-devel
[-- Attachment #1: Type: text/plain, Size: 4025 bytes --]
Hi Volker,
> 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.
>
Oh, I missed these. Thank you for pointing it out.
> 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.
Fair enough.
> > 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.
>
Thank you for pointing it out! Now, I understand better!
> 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
>
I rethink and want to say that a low sample rate could be a problem. Have
checked the missed patch
https://lists.nongnu.org/archive/html/qemu-devel/2022-12/msg02895.html, I
think you have addressed my concern. Thank all your suggestions again!
Let's close this discussion and I will close the issue.
Best,
Qiang
[-- Attachment #2: Type: text/html, Size: 6001 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread