[PATCH v4 0/1] MR10326: mmdevapi: Avoid copying more memory than reserved in validate_fmt (ASan).
The function `validate_fmt` may receive a pointer which got just reserved memory for the `WAVEFORMATEX`. Unfortunately the assignment `fmt2 = *fmt` always copies the size of `WAVEFORMATEXTENSIBLE`. CC: @giomasce (I currently cannot point to a specific commit, but this may also be related to the recent changes?) <details> <summary>ASan details [gitlab CI](https://gitlab.winehq.org/bernhardu/wine/-/jobs/237050#L2464)</summary> ``` ==qcap_test.exe==728==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x0022c122 at pc 0x785f4342 bp 0x0022bf4c sp 0x0022bb1c READ of size 40 at 0x0022c122 thread T0 #0 0x785f4341 in __asan_memcpy /home/runner/work/llvm-mingw/llvm-mingw/llvm-project/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp:65:3 #1 0x778957a5 in validate_fmt /builds/bernhardu/wine/build64/../dlls/mmdevapi/client.c:363:33 #2 0x77896d1e in client_IsFormatSupported /builds/bernhardu/wine/build64/../dlls/mmdevapi/client.c:881:10 #3 0x783e3624 in IAudioClient_IsFormatSupported /builds/bernhardu/wine/build64/include/audioclient.h:406:12 #4 0x783e5cd7 in WINMM_TestFormat /builds/bernhardu/wine/build64/../dlls/winmm/waveform.c:454:10 #5 0x783e5928 in WINMM_GetSupportedFormats /builds/bernhardu/wine/build64/../dlls/winmm/waveform.c:504:14 #6 0x783e553b in WINMM_InitMMDevice /builds/bernhardu/wine/build64/../dlls/winmm/waveform.c:524:35 #7 0x783e4e9b in WINMM_EnumDevices /builds/bernhardu/wine/build64/../dlls/winmm/waveform.c:610:17 #8 0x783d30e8 in WINMM_InitMMDevices /builds/bernhardu/wine/build64/../dlls/winmm/waveform.c:856:10 #9 0x7be4a996 in RtlRunOnceExecuteOnce /builds/bernhardu/wine/build64/../dlls/ntdll/sync.c:462:10 ... Address 0x0022c122 is located in stack of thread T0 at offset 34 in frame #0 0x783e5c0f in WINMM_TestFormat /builds/bernhardu/wine/build64/../dlls/winmm/waveform.c:442 This frame has 2 object(s): [16, 34) 'fmt' (line 443) <== Memory access at offset 34 overflows this variable [80, 84) 'junk' (line 443) HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork (longjmp, SEH and C++ exceptions *are* supported) SUMMARY: AddressSanitizer: stack-buffer-overflow /builds/bernhardu/wine/build64/../dlls/mmdevapi/client.c:363:33 in validate_fmt ``` </details> -- v4: mmdevapi: Avoid copying more memory than reserved in validate_fmt (ASan). https://gitlab.winehq.org/wine/wine/-/merge_requests/10326
From: Bernhard Übelacker <bernhardu@mailbox.org> Co-authored-by: Giovanni Mascellani <gmascellani@codeweavers.com> --- dlls/mmdevapi/client.c | 16 +++++++++++----- dlls/mmdevapi/tests/render.c | 16 +++++++++++----- dlls/winmm/tests/wave.c | 15 ++++++++++----- 3 files changed, 32 insertions(+), 15 deletions(-) diff --git a/dlls/mmdevapi/client.c b/dlls/mmdevapi/client.c index 4c23905aad8..d7e05a68215 100644 --- a/dlls/mmdevapi/client.c +++ b/dlls/mmdevapi/client.c @@ -360,12 +360,14 @@ skip: HRESULT validate_fmt(const WAVEFORMATEXTENSIBLE *fmt, BOOL compatible) { - WAVEFORMATEXTENSIBLE fmt2 = *fmt; + WAVEFORMATEXTENSIBLE fmt2; HRESULT ret; /* Reduce non-extensible formats to extensible ones. */ - if (fmt2.Format.wFormatTag != WAVE_FORMAT_EXTENSIBLE) + if (fmt->Format.wFormatTag != WAVE_FORMAT_EXTENSIBLE) { + fmt2.Format = fmt->Format; + switch (fmt2.Format.wFormatTag) { case WAVE_FORMAT_PCM: fmt2.SubFormat = KSDATAFORMAT_SUBTYPE_PCM; break; @@ -380,10 +382,14 @@ HRESULT validate_fmt(const WAVEFORMATEXTENSIBLE *fmt, BOOL compatible) fmt2.Samples.wValidBitsPerSample = fmt2.Format.wBitsPerSample; fmt2.Format.cbSize = sizeof(fmt2) - sizeof(fmt2.Format); } + else + { + if (fmt->Format.cbSize < sizeof(fmt2) - sizeof(fmt2.Format)) + return E_INVALIDARG; + fmt2 = *fmt; + } - if (fmt2.Format.cbSize < sizeof(fmt2) - sizeof(fmt2.Format)) - ret = E_INVALIDARG; - else if (fmt2.Format.nChannels == 0 || fmt2.Format.nSamplesPerSec == 0) + if (fmt2.Format.nChannels == 0 || fmt2.Format.nSamplesPerSec == 0) ret = E_INVALIDARG; else if (fmt2.Format.nBlockAlign != fmt2.Format.nChannels * fmt2.Format.wBitsPerSample / 8) ret = E_INVALIDARG; diff --git a/dlls/mmdevapi/tests/render.c b/dlls/mmdevapi/tests/render.c index 6fa1465a5f3..192ef180139 100644 --- a/dlls/mmdevapi/tests/render.c +++ b/dlls/mmdevapi/tests/render.c @@ -790,12 +790,14 @@ void fill_wave_formats(const WAVEFORMATEXTENSIBLE *base_fmt) /* Identical to dlls/mmdevapi/client.c. */ HRESULT validate_fmt(const WAVEFORMATEXTENSIBLE *fmt, BOOL compatible) { - WAVEFORMATEXTENSIBLE fmt2 = *fmt; + WAVEFORMATEXTENSIBLE fmt2; HRESULT ret; /* Reduce non-extensible formats to extensible ones. */ - if (fmt2.Format.wFormatTag != WAVE_FORMAT_EXTENSIBLE) + if (fmt->Format.wFormatTag != WAVE_FORMAT_EXTENSIBLE) { + fmt2.Format = fmt->Format; + switch (fmt2.Format.wFormatTag) { case WAVE_FORMAT_PCM: fmt2.SubFormat = KSDATAFORMAT_SUBTYPE_PCM; break; @@ -810,10 +812,14 @@ HRESULT validate_fmt(const WAVEFORMATEXTENSIBLE *fmt, BOOL compatible) fmt2.Samples.wValidBitsPerSample = fmt2.Format.wBitsPerSample; fmt2.Format.cbSize = sizeof(fmt2) - sizeof(fmt2.Format); } + else + { + if (fmt->Format.cbSize < sizeof(fmt2) - sizeof(fmt2.Format)) + return E_INVALIDARG; + fmt2 = *fmt; + } - if (fmt2.Format.cbSize < sizeof(fmt2) - sizeof(fmt2.Format)) - ret = E_INVALIDARG; - else if (fmt2.Format.nChannels == 0 || fmt2.Format.nSamplesPerSec == 0) + if (fmt2.Format.nChannels == 0 || fmt2.Format.nSamplesPerSec == 0) ret = E_INVALIDARG; else if (fmt2.Format.nBlockAlign != fmt2.Format.nChannels * fmt2.Format.wBitsPerSample / 8) ret = E_INVALIDARG; diff --git a/dlls/winmm/tests/wave.c b/dlls/winmm/tests/wave.c index b0b7a6a3123..50a656a741d 100644 --- a/dlls/winmm/tests/wave.c +++ b/dlls/winmm/tests/wave.c @@ -1901,13 +1901,14 @@ static void test_PlaySound(void) static MMRESULT validate_fmt(const WAVEFORMATEXTENSIBLE *fmt, BOOL direct) { - WAVEFORMATEXTENSIBLE fmt2 = *fmt; + WAVEFORMATEXTENSIBLE fmt2; BOOL extensible = TRUE; MMRESULT ret; /* Reduce non-extensible formats to extensible ones. */ - if (fmt2.Format.wFormatTag != WAVE_FORMAT_EXTENSIBLE) + if (fmt->Format.wFormatTag != WAVE_FORMAT_EXTENSIBLE) { + fmt2.Format = fmt->Format; extensible = FALSE; switch (fmt2.Format.wFormatTag) @@ -1926,10 +1927,14 @@ static MMRESULT validate_fmt(const WAVEFORMATEXTENSIBLE *fmt, BOOL direct) fmt2.Samples.wValidBitsPerSample = fmt2.Format.wBitsPerSample; fmt2.Format.cbSize = sizeof(fmt2) - sizeof(fmt2.Format); } + else + { + if (fmt->Format.cbSize < sizeof(fmt2) - sizeof(fmt2.Format)) + return WAVERR_BADFORMAT; + fmt2 = *fmt; + } - if (fmt2.Format.cbSize < sizeof(fmt2) - sizeof(fmt2.Format)) - ret = direct ? MMSYSERR_INVALPARAM : WAVERR_BADFORMAT; - else if ((fmt2.Format.nChannels == 0) || fmt2.Format.nSamplesPerSec == 0) + if ((fmt2.Format.nChannels == 0) || fmt2.Format.nSamplesPerSec == 0) ret = WAVERR_BADFORMAT; else if (fmt2.Format.nBlockAlign != fmt2.Format.nChannels * fmt2.Format.wBitsPerSample / 8) ret = WAVERR_BADFORMAT; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10326
v4: - Use in `validate_fmt` in wave.c `WAVERR_BADFORMAT` instead of `E_INVALIDARG`. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10326#note_133365
This merge request was approved by Huw Davies. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10326
participants (2)
-
Bernhard Übelacker -
Huw Davies (＠huw)