| Message ID | 20221225121357.498040-1-cyruscyliu@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/audio/c97: fix abort in audio_calloc() | expand |
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); >
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);
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);
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);
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
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);
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(-)