[PULL 0/2] Audio 20210624 patches

[PULL 0/2] Audio 20210624 patches

[Gerd Hoffmann]

The following changes since commit b22726abdfa54592d6ad88f65b0297c0e8b363e2:

  Merge remote-tracking branch 'remotes/vivier2/tags/linux-user-for-6.1-pull-request' into staging (2021-06-22 16:07:53 +0100)

are available in the Git repository at:

  git://git.kraxel.org/qemu tags/audio-20210624-pull-request

for you to fetch changes up to 60e543f5ce46d4a90a95963b3bab5c7d13a2aaa9:

  hw/audio/sb16: Restrict I/O sampling rate range for command 41h/42h (2021-06-24 11:42:54 +0200)

----------------------------------------------------------------
audio: bugfixes

----------------------------------------------------------------

Akihiko Odaki (1):
  coreaudio: Lock only the buffer

Qiang Liu (1):
  hw/audio/sb16: Restrict I/O sampling rate range for command 41h/42h

 audio/coreaudio.c            | 102 ++++++++++++++---------------------
 hw/audio/sb16.c              |  31 ++++++-----
 tests/qtest/fuzz-sb16-test.c |  17 ++++++
 3 files changed, 77 insertions(+), 73 deletions(-)

-- 
2.31.1

[PULL 1/2] coreaudio: Lock only the buffer

[Gerd Hoffmann]

From: Akihiko Odaki <akihiko.odaki@gmail.com>

On macOS 11.3.1, Core Audio calls AudioDeviceIOProc after calling an
internal function named HALB_Mutex::Lock(), which locks a mutex in
HALB_IOThread::Entry(void*). HALB_Mutex::Lock() is also called in
AudioObjectGetPropertyData, which is called by coreaudio driver.
Therefore, a deadlock will occur if coreaudio driver calls
AudioObjectGetPropertyData while holding a lock for a mutex and tries
to lock the same mutex in AudioDeviceIOProc.

audioDeviceIOProc, which implements AudioDeviceIOProc in coreaudio
driver, requires an exclusive access for the device configuration and
the buffer. Fortunately, a mutex is necessary only for the buffer in
audioDeviceIOProc because a change for the device configuration occurs
only before setting up AudioDeviceIOProc or after stopping the playback
with AudioDeviceStop.

With this change, the mutex owned by the driver will only be used for
the buffer, and the device configuration change will be protected with
the implicit iothread mutex.

Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
Message-id: 20210622201740.38005-1-akihiko.odaki@gmail.com
Message-Id: <20210622201740.38005-1-akihiko.odaki@gmail.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 audio/coreaudio.c | 102 +++++++++++++++++++---------------------------
 1 file changed, 41 insertions(+), 61 deletions(-)

diff --git a/audio/coreaudio.c b/audio/coreaudio.c
index f570e1ee60ef..d8a21d3e5075 100644
--- a/audio/coreaudio.c
+++ b/audio/coreaudio.c
@@ -26,6 +26,7 @@
 #include <CoreAudio/CoreAudio.h>
 #include <pthread.h>            /* pthread_X */
 
+#include "qemu/main-loop.h"
 #include "qemu/module.h"
 #include "audio.h"
 
@@ -34,7 +35,7 @@
 
 typedef struct coreaudioVoiceOut {
     HWVoiceOut hw;
-    pthread_mutex_t mutex;
+    pthread_mutex_t buf_mutex;
     AudioDeviceID outputDeviceID;
     int frameSizeSetting;
     uint32_t bufferCount;
@@ -241,11 +242,11 @@ static void GCC_FMT_ATTR (3, 4) coreaudio_logerr2 (
 #define coreaudio_playback_logerr(status, ...) \
     coreaudio_logerr2(status, "playback", __VA_ARGS__)
 
-static int coreaudio_lock (coreaudioVoiceOut *core, const char *fn_name)
+static int coreaudio_buf_lock (coreaudioVoiceOut *core, const char *fn_name)
 {
     int err;
 
-    err = pthread_mutex_lock (&core->mutex);
+    err = pthread_mutex_lock (&core->buf_mutex);
     if (err) {
         dolog ("Could not lock voice for %s\nReason: %s\n",
                fn_name, strerror (err));
@@ -254,11 +255,11 @@ static int coreaudio_lock (coreaudioVoiceOut *core, const char *fn_name)
     return 0;
 }
 
-static int coreaudio_unlock (coreaudioVoiceOut *core, const char *fn_name)
+static int coreaudio_buf_unlock (coreaudioVoiceOut *core, const char *fn_name)
 {
     int err;
 
-    err = pthread_mutex_unlock (&core->mutex);
+    err = pthread_mutex_unlock (&core->buf_mutex);
     if (err) {
         dolog ("Could not unlock voice for %s\nReason: %s\n",
                fn_name, strerror (err));
@@ -273,13 +274,13 @@ static int coreaudio_unlock (coreaudioVoiceOut *core, const char *fn_name)
         coreaudioVoiceOut *core = (coreaudioVoiceOut *) hw;     \
         ret_type ret;                                           \
                                                                 \
-        if (coreaudio_lock(core, "coreaudio_" #name)) {         \
+        if (coreaudio_buf_lock(core, "coreaudio_" #name)) {         \
             return 0;                                           \
         }                                                       \
                                                                 \
         ret = glue(audio_generic_, name)args;                   \
                                                                 \
-        coreaudio_unlock(core, "coreaudio_" #name);             \
+        coreaudio_buf_unlock(core, "coreaudio_" #name);             \
         return ret;                                             \
     }
 COREAUDIO_WRAPPER_FUNC(get_buffer_out, void *, (HWVoiceOut *hw, size_t *size),
@@ -291,7 +292,10 @@ COREAUDIO_WRAPPER_FUNC(write, size_t, (HWVoiceOut *hw, void *buf, size_t size),
                        (hw, buf, size))
 #undef COREAUDIO_WRAPPER_FUNC
 
-/* callback to feed audiooutput buffer */
+/*
+ * callback to feed audiooutput buffer. called without iothread lock.
+ * allowed to lock "buf_mutex", but disallowed to have any other locks.
+ */
 static OSStatus audioDeviceIOProc(
     AudioDeviceID inDevice,
     const AudioTimeStamp *inNow,
@@ -307,13 +311,13 @@ static OSStatus audioDeviceIOProc(
     coreaudioVoiceOut *core = (coreaudioVoiceOut *) hwptr;
     size_t len;
 
-    if (coreaudio_lock (core, "audioDeviceIOProc")) {
+    if (coreaudio_buf_lock (core, "audioDeviceIOProc")) {
         inInputTime = 0;
         return 0;
     }
 
     if (inDevice != core->outputDeviceID) {
-        coreaudio_unlock (core, "audioDeviceIOProc(old device)");
+        coreaudio_buf_unlock (core, "audioDeviceIOProc(old device)");
         return 0;
     }
 
@@ -323,7 +327,7 @@ static OSStatus audioDeviceIOProc(
     /* if there are not enough samples, set signal and return */
     if (pending_frames < frameCount) {
         inInputTime = 0;
-        coreaudio_unlock (core, "audioDeviceIOProc(empty)");
+        coreaudio_buf_unlock (core, "audioDeviceIOProc(empty)");
         return 0;
     }
 
@@ -345,7 +349,7 @@ static OSStatus audioDeviceIOProc(
         out += write_len;
     }
 
-    coreaudio_unlock (core, "audioDeviceIOProc");
+    coreaudio_buf_unlock (core, "audioDeviceIOProc");
     return 0;
 }
 
@@ -438,7 +442,16 @@ static OSStatus init_out_device(coreaudioVoiceOut *core)
         return status;
     }
 
-    /* set Callback */
+    /*
+     * set Callback.
+     *
+     * On macOS 11.3.1, Core Audio calls AudioDeviceIOProc after calling an
+     * internal function named HALB_Mutex::Lock(), which locks a mutex in
+     * HALB_IOThread::Entry(void*). HALB_Mutex::Lock() is also called in
+     * AudioObjectGetPropertyData, which is called by coreaudio driver.
+     * Therefore, the specified callback must be designed to avoid a deadlock
+     * with the callers of AudioObjectGetPropertyData.
+     */
     core->ioprocid = NULL;
     status = AudioDeviceCreateIOProcID(core->outputDeviceID,
                                        audioDeviceIOProc,
@@ -521,6 +534,7 @@ static void update_device_playback_state(coreaudioVoiceOut *core)
     }
 }
 
+/* called without iothread lock. */
 static OSStatus handle_voice_change(
     AudioObjectID in_object_id,
     UInt32 in_number_addresses,
@@ -530,9 +544,7 @@ static OSStatus handle_voice_change(
     OSStatus status;
     coreaudioVoiceOut *core = in_client_data;
 
-    if (coreaudio_lock(core, __func__)) {
-        abort();
-    }
+    qemu_mutex_lock_iothread();
 
     if (core->outputDeviceID) {
         fini_out_device(core);
@@ -543,7 +555,7 @@ static OSStatus handle_voice_change(
         update_device_playback_state(core);
     }
 
-    coreaudio_unlock (core, __func__);
+    qemu_mutex_unlock_iothread();
     return status;
 }
 
@@ -558,14 +570,10 @@ static int coreaudio_init_out(HWVoiceOut *hw, struct audsettings *as,
     struct audsettings obt_as;
 
     /* create mutex */
-    err = pthread_mutex_init(&core->mutex, NULL);
+    err = pthread_mutex_init(&core->buf_mutex, NULL);
     if (err) {
         dolog("Could not create mutex\nReason: %s\n", strerror (err));
-        goto mutex_error;
-    }
-
-    if (coreaudio_lock(core, __func__)) {
-        goto lock_error;
+        return -1;
     }
 
     obt_as = *as;
@@ -584,37 +592,21 @@ static int coreaudio_init_out(HWVoiceOut *hw, struct audsettings *as,
     if (status != kAudioHardwareNoError) {
         coreaudio_playback_logerr (status,
                                    "Could not listen to voice property change\n");
-        goto listener_error;
+        return -1;
     }
 
     if (init_out_device(core)) {
-        goto device_error;
+        status = AudioObjectRemovePropertyListener(kAudioObjectSystemObject,
+                                                   &voice_addr,
+                                                   handle_voice_change,
+                                                   core);
+        if (status != kAudioHardwareNoError) {
+            coreaudio_playback_logerr(status,
+                                      "Could not remove voice property change listener\n");
+        }
     }
 
-    coreaudio_unlock(core, __func__);
     return 0;
-
-device_error:
-    status = AudioObjectRemovePropertyListener(kAudioObjectSystemObject,
-                                               &voice_addr,
-                                               handle_voice_change,
-                                               core);
-    if (status != kAudioHardwareNoError) {
-        coreaudio_playback_logerr(status,
-                                  "Could not remove voice property change listener\n");
-    }
-
-listener_error:
-    coreaudio_unlock(core, __func__);
-
-lock_error:
-    err = pthread_mutex_destroy(&core->mutex);
-    if (err) {
-        dolog("Could not destroy mutex\nReason: %s\n", strerror (err));
-    }
-
-mutex_error:
-    return -1;
 }
 
 static void coreaudio_fini_out (HWVoiceOut *hw)
@@ -623,10 +615,6 @@ static void coreaudio_fini_out (HWVoiceOut *hw)
     int err;
     coreaudioVoiceOut *core = (coreaudioVoiceOut *) hw;
 
-    if (coreaudio_lock(core, __func__)) {
-        abort();
-    }
-
     status = AudioObjectRemovePropertyListener(kAudioObjectSystemObject,
                                                &voice_addr,
                                                handle_voice_change,
@@ -637,10 +625,8 @@ static void coreaudio_fini_out (HWVoiceOut *hw)
 
     fini_out_device(core);
 
-    coreaudio_unlock(core, __func__);
-
     /* destroy mutex */
-    err = pthread_mutex_destroy(&core->mutex);
+    err = pthread_mutex_destroy(&core->buf_mutex);
     if (err) {
         dolog("Could not destroy mutex\nReason: %s\n", strerror (err));
     }
@@ -650,14 +636,8 @@ static void coreaudio_enable_out(HWVoiceOut *hw, bool enable)
 {
     coreaudioVoiceOut *core = (coreaudioVoiceOut *) hw;
 
-    if (coreaudio_lock(core, __func__)) {
-        abort();
-    }
-
     core->enabled = enable;
     update_device_playback_state(core);
-
-    coreaudio_unlock(core, __func__);
 }
 
 static void *coreaudio_audio_init(Audiodev *dev)
-- 
2.31.1

[PULL 2/2] hw/audio/sb16: Restrict I/O sampling rate range for command 41h/42h

[Gerd Hoffmann]

From: Qiang Liu <cyruscyliu@gmail.com>

The I/O sampling rate range is enforced to 5000 to 45000HZ according to
commit a2cd86a9. Setting I/O sampling rate with command 41h/42h, a guest
user can break this assumption and trigger an assertion in audio_calloc
via command 0xd4. This patch restricts the I/O sampling rate range for
command 41h/42h.

Fixes: 85571bc7415 ("audio merge (malc)")
Signed-off-by: Qiang Liu <cyruscyliu@gmail.com>
Message-Id: <1624502687-5214-1-git-send-email-cyruscyliu@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/audio/sb16.c              | 31 +++++++++++++++++++------------
 tests/qtest/fuzz-sb16-test.c | 17 +++++++++++++++++
 2 files changed, 36 insertions(+), 12 deletions(-)

diff --git a/hw/audio/sb16.c b/hw/audio/sb16.c
index 5cf121fe3639..60f1f75e3ac8 100644
--- a/hw/audio/sb16.c
+++ b/hw/audio/sb16.c
@@ -229,6 +229,23 @@ static void continue_dma8 (SB16State *s)
     control (s, 1);
 }
 
+static inline int restrict_sampling_rate(int freq)
+{
+    if (freq < SAMPLE_RATE_MIN) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "sampling range too low: %d, increasing to %u\n",
+                      freq, SAMPLE_RATE_MIN);
+        return SAMPLE_RATE_MIN;
+    } else if (freq > SAMPLE_RATE_MAX) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "sampling range too high: %d, decreasing to %u\n",
+                      freq, SAMPLE_RATE_MAX);
+        return SAMPLE_RATE_MAX;
+    } else {
+        return freq;
+    }
+}
+
 static void dma_cmd8 (SB16State *s, int mask, int dma_len)
 {
     s->fmt = AUDIO_FORMAT_U8;
@@ -244,17 +261,7 @@ static void dma_cmd8 (SB16State *s, int mask, int dma_len)
         int tmp = (256 - s->time_const);
         s->freq = (1000000 + (tmp / 2)) / tmp;
     }
-    if (s->freq < SAMPLE_RATE_MIN) {
-        qemu_log_mask(LOG_GUEST_ERROR,
-                      "sampling range too low: %d, increasing to %u\n",
-                      s->freq, SAMPLE_RATE_MIN);
-        s->freq = SAMPLE_RATE_MIN;
-    } else if (s->freq > SAMPLE_RATE_MAX) {
-        qemu_log_mask(LOG_GUEST_ERROR,
-                      "sampling range too high: %d, decreasing to %u\n",
-                      s->freq, SAMPLE_RATE_MAX);
-        s->freq = SAMPLE_RATE_MAX;
-    }
+    s->freq = restrict_sampling_rate(s->freq);
 
     if (dma_len != -1) {
         s->block_size = dma_len << s->fmt_stereo;
@@ -768,7 +775,7 @@ static void complete (SB16State *s)
              * and FT2 sets output freq with this (go figure).  Compare:
              * http://homepages.cae.wisc.edu/~brodskye/sb16doc/sb16doc.html#SamplingRate
              */
-            s->freq = dsp_get_hilo (s);
+            s->freq = restrict_sampling_rate(dsp_get_hilo(s));
             ldebug ("set freq %d\n", s->freq);
             break;
 
diff --git a/tests/qtest/fuzz-sb16-test.c b/tests/qtest/fuzz-sb16-test.c
index 51030cd7dc40..f47a8bcdbd91 100644
--- a/tests/qtest/fuzz-sb16-test.c
+++ b/tests/qtest/fuzz-sb16-test.c
@@ -37,6 +37,22 @@ static void test_fuzz_sb16_0x91(void)
     qtest_quit(s);
 }
 
+/*
+ * This used to trigger the assert in audio_calloc
+ * through command 0xd4
+ */
+static void test_fuzz_sb16_0xd4(void)
+{
+    QTestState *s = qtest_init("-M pc -display none "
+                               "-device sb16,audiodev=none "
+                               "-audiodev id=none,driver=none");
+    qtest_outb(s, 0x22c, 0x41);
+    qtest_outb(s, 0x22c, 0x00);
+    qtest_outb(s, 0x22c, 0x14);
+    qtest_outb(s, 0x22c, 0xd4);
+    qtest_quit(s);
+}
+
 int main(int argc, char **argv)
 {
     const char *arch = qtest_get_arch();
@@ -46,6 +62,7 @@ int main(int argc, char **argv)
    if (strcmp(arch, "i386") == 0) {
         qtest_add_func("fuzz/test_fuzz_sb16/1c", test_fuzz_sb16_0x1c);
         qtest_add_func("fuzz/test_fuzz_sb16/91", test_fuzz_sb16_0x91);
+        qtest_add_func("fuzz/test_fuzz_sb16/d4", test_fuzz_sb16_0xd4);
    }
 
    return g_test_run();
-- 
2.31.1

Re: [PULL 0/2] Audio 20210624 patches

[no-reply]

Patchew URL: https://patchew.org/QEMU/20210624121639.2422367-1-kraxel@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210624121639.2422367-1-kraxel@redhat.com
Subject: [PULL 0/2] Audio 20210624 patches

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20210624121639.2422367-1-kraxel@redhat.com -> patchew/20210624121639.2422367-1-kraxel@redhat.com
Switched to a new branch 'test'
b397205 hw/audio/sb16: Restrict I/O sampling rate range for command 41h/42h
8b66bf4 coreaudio: Lock only the buffer

=== OUTPUT BEGIN ===
1/2 Checking commit 8b66bf451269 (coreaudio: Lock only the buffer)
ERROR: space prohibited between function name and open parenthesis '('
#61: FILE: audio/coreaudio.c:245:
+static int coreaudio_buf_lock (coreaudioVoiceOut *core, const char *fn_name)

ERROR: space prohibited between function name and open parenthesis '('
#66: FILE: audio/coreaudio.c:249:
+    err = pthread_mutex_lock (&core->buf_mutex);

ERROR: space prohibited between function name and open parenthesis '('
#75: FILE: audio/coreaudio.c:258:
+static int coreaudio_buf_unlock (coreaudioVoiceOut *core, const char *fn_name)

ERROR: space prohibited between function name and open parenthesis '('
#80: FILE: audio/coreaudio.c:262:
+    err = pthread_mutex_unlock (&core->buf_mutex);

ERROR: space prohibited between function name and open parenthesis '('
#117: FILE: audio/coreaudio.c:314:
+    if (coreaudio_buf_lock (core, "audioDeviceIOProc")) {

ERROR: space prohibited between function name and open parenthesis '('
#124: FILE: audio/coreaudio.c:320:
+        coreaudio_buf_unlock (core, "audioDeviceIOProc(old device)");

ERROR: space prohibited between function name and open parenthesis '('
#133: FILE: audio/coreaudio.c:330:
+        coreaudio_buf_unlock (core, "audioDeviceIOProc(empty)");

ERROR: space prohibited between function name and open parenthesis '('
#142: FILE: audio/coreaudio.c:352:
+    coreaudio_buf_unlock (core, "audioDeviceIOProc");

total: 8 errors, 0 warnings, 237 lines checked

Patch 1/2 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

2/2 Checking commit b39720569bf6 (hw/audio/sb16: Restrict I/O sampling rate range for command 41h/42h)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210624121639.2422367-1-kraxel@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PULL 0/2] Audio 20210624 patches

[Peter Maydell]

On Thu, 24 Jun 2021 at 13:18, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> The following changes since commit b22726abdfa54592d6ad88f65b0297c0e8b363e2:
>
>   Merge remote-tracking branch 'remotes/vivier2/tags/linux-user-for-6.1-pull-request' into staging (2021-06-22 16:07:53 +0100)
>
> are available in the Git repository at:
>
>   git://git.kraxel.org/qemu tags/audio-20210624-pull-request
>
> for you to fetch changes up to 60e543f5ce46d4a90a95963b3bab5c7d13a2aaa9:
>
>   hw/audio/sb16: Restrict I/O sampling rate range for command 41h/42h (2021-06-24 11:42:54 +0200)
>
> ----------------------------------------------------------------
> audio: bugfixes
>


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/6.1
for any user-visible changes.

-- PMM