Skip to contentSkip to conversation

ALSA: pcm: Don't WARN on NULL dma_area in the default mmap fault handler

PATCH In review Dmitriy Chumachenko 2 participants 2 messages Oct 30 – Oct 31 Sound

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 ALSA: pcm: Don't WARN on NULL dma_area in the default mmap fault handler

During fuzz testing, the following issue was discovered.

WARNING: CPU: 2 PID: 1734 at sound/core/pcm_native.c:3835 
snd_pcm_mmap_data_fault+0x47f/0x530 sound/core/pcm_native.c:3835

Modules linked in:
CPU: 2 PID: 1734 Comm: syz.1.352 Not tainted 6.1.187-syzkaller-00593-g063a81552262 #0
RIP: 0010:snd_pcm_mmap_data_fault+0x47f/0x530 sound/core/pcm_native.c:3835

Call Trace:
 <TASK>
 __do_fault+0x10d/0x480 mm/memory.c:4280
 do_cow_fault mm/memory.c:4661 [inline]
 do_fault+0xa7a/0x1100 mm/memory.c:4762
 handle_pte_fault mm/memory.c:5031 [inline]
 __handle_mm_fault+0xdf3/0x2e40 mm/memory.c:5173
 handle_mm_fault+0x2da/0xa60 mm/memory.c:5294
 do_user_addr_fault+0x4aa/0x1050 arch/x86/mm/fault.c:1338
 handle_page_fault arch/x86/mm/fault.c:1429 [inline]
 exc_page_fault+0x98/0x1b0 arch/x86/mm/fault.c:1482
 asm_exc_page_fault+0x22/0x30 arch/x86/include/asm/idtentry.h:608

snd_pcm_mmap_data_fault() guards the default RAM-buffer path with WARN_ON_ONCE(!runtime->dma_area). As commit d2913a07d903 notes, a driver may allow mmap before dma_area is set up, so this state is reachable from user space: mmap the PCM data region and fault it while the buffer is not allocated. The WARN then fires on a user-triggerable path and panics a kernel with panic_on_warn.

Drop the WARN; the NULL check and VM_FAULT_SIGBUS return stay, which is the correct silent handling.

Fixes d2913a07d903 ("ALSA: pcm: Add sanity NULL check for the default mmap fault handler")
---
 sound/core/pcm_native.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
sound/core/pcm_native.c +1 -1
index fcb5f176f0df..dd640d90f6a3 100644
@@ -3832,7 +3832,7 @@ static vm_fault_t snd_pcm_mmap_data_fault(struct vm_fault *vmf)
 	if (substream->ops->page)
 		page = substream->ops->page(substream, offset);
 	else if (!snd_pcm_get_dma_buf(substream)) {
-		if (WARN_ON_ONCE(!runtime->dma_area))
+		if (!runtime->dma_area)
 			return VM_FAULT_SIGBUS;
 		page = virt_to_page(runtime->dma_area + offset);
 	} else
Signature · 1 line
2.49.0
#1 lore raw
Quoted 30 lines · Dmitriy Chumachenko

During fuzz testing, the following issue was discovered.

WARNING: CPU: 2 PID: 1734 at sound/core/pcm_native.c:3835 
snd_pcm_mmap_data_fault+0x47f/0x530 sound/core/pcm_native.c:3835

Modules linked in:
CPU: 2 PID: 1734 Comm: syz.1.352 Not tainted 6.1.187-syzkaller-00593-g063a81552262 #0
RIP: 0010:snd_pcm_mmap_data_fault+0x47f/0x530 sound/core/pcm_native.c:3835

Call Trace:
 <TASK>
 __do_fault+0x10d/0x480 mm/memory.c:4280
 do_cow_fault mm/memory.c:4661 [inline]
 do_fault+0xa7a/0x1100 mm/memory.c:4762
 handle_pte_fault mm/memory.c:5031 [inline]
 __handle_mm_fault+0xdf3/0x2e40 mm/memory.c:5173
 handle_mm_fault+0x2da/0xa60 mm/memory.c:5294
 do_user_addr_fault+0x4aa/0x1050 arch/x86/mm/fault.c:1338
 handle_page_fault arch/x86/mm/fault.c:1429 [inline]
 exc_page_fault+0x98/0x1b0 arch/x86/mm/fault.c:1482
 asm_exc_page_fault+0x22/0x30 arch/x86/include/asm/idtentry.h:608

snd_pcm_mmap_data_fault() guards the default RAM-buffer path with WARN_ON_ONCE(!runtime->dma_area). As commit d2913a07d903 notes, a driver may allow mmap before dma_area is set up, so this state is reachable from user space: mmap the PCM data region and fault it while the buffer is not allocated. The WARN then fires on a user-triggerable path and panics a kernel with panic_on_warn.

Quoted 5 lines

Drop the WARN; the NULL check and VM_FAULT_SIGBUS return stay, which is the correct silent handling.

Fixes d2913a07d903 ("ALSA: pcm: Add sanity NULL check for the default mmap fault handler")

The WARN_ON() was put there because it shouldn't happen, and dropping WARN_ON() is likely an incorrect "solution".

Could you rather investigate how this is triggered (under which condition with which driver)? That should be addressed instead of just silencing it.

thanks,

Takashi