Re: [syzbot] KASAN: use-after-free Read in __snd_rawmidi_transmit_peek

From: Takashi Iwai <tiwai@suse.de>
To: syzbot <syzbot+70e777a39907d6d5fd0a@syzkaller.appspotmail.com>
Cc: alsa-devel@alsa-project.org, coding@diwic.se,
	colin.king@intel.com, linux-kernel@vger.kernel.org,
	perex@perex.cz, syzkaller-bugs@googlegroups.com, tiwai@suse.com
Subject: Re: [syzbot] KASAN: use-after-free Read in __snd_rawmidi_transmit_peek
Date: Tue, 19 Apr 2022 11:01:47 +0200	[thread overview]
Message-ID: <s5hk0bl1m6c.wl-tiwai@suse.de> (raw)
In-Reply-To: <00000000000011555605dceaff03@google.com>

On Mon, 18 Apr 2022 12:12:23 +0200,
syzbot wrote:
> 
> Hello,
> 
> syzbot found the following issue on:
> 
> HEAD commit:    a19944809fe9 Merge tag 'hardening-v5.18-rc3' of git://git...
> git tree:       upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=16a40ae0f00000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=eb177500e563582f
> dashboard link: https://syzkaller.appspot.com/bug?extid=70e777a39907d6d5fd0a
> compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=1590dfa8f00000
> 
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+70e777a39907d6d5fd0a@syzkaller.appspotmail.com
> 
> ==================================================================
> BUG: KASAN: use-after-free in __snd_rawmidi_transmit_peek+0x261/0x360 sound/core/rawmidi.c:1286

Looks like a leftover work.  The fix patch is below.


Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] ALSA: usb-audio: Fix potential use-after-free at releasing
 MIDI streams

As recently spotted by syzbot, the USB MIDI code may leave a work that
handles the pending output queue even after a stream gets released
(either internally via sequencer or via device files), which may lead
to use-after-free.  We have already a proper drop of pending URBs but
this is executed only at the device disconnection.

This patch moves the URB clean-up code into both the code path that is
called at releasing, namely, snd_usb_midi_in_endpoint_delete() and
snd_usb_midi_out_endpoint_clear() functions, so that the pending URBs
and work are cleared properly before releasing the resources.

Reported-by: syzbot+70e777a39907d6d5fd0a@syzkaller.appspotmail.com
Cc: <stable@vger.kernel.org>
Link: https://lore.kernel.org/r/00000000000011555605dceaff03@google.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/usb/midi.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/sound/usb/midi.c b/sound/usb/midi.c
index 2c01649c70f6..8e5281f7f1fc 100644
--- a/sound/usb/midi.c
+++ b/sound/usb/midi.c
@@ -1247,6 +1247,10 @@ static void snd_usbmidi_in_endpoint_delete(struct snd_usb_midi_in_endpoint *ep)
 {
 	unsigned int i;
 
+	for (i = 0; i < INPUT_URBS; ++i)
+		if (ep->urbs[i])
+			usb_kill_urb(ep->urbs[i]);
+
 	for (i = 0; i < INPUT_URBS; ++i)
 		if (ep->urbs[i])
 			free_urb_and_buffer(ep->umidi, ep->urbs[i],
@@ -1327,6 +1331,18 @@ static void snd_usbmidi_out_endpoint_clear(struct snd_usb_midi_out_endpoint *ep)
 {
 	unsigned int i;
 
+	cancel_work_sync(&ep->work);
+	for (i = 0; i < OUTPUT_URBS; ++i)
+		if (ep->urbs[i].urb)
+			usb_kill_urb(ep->urbs[i].urb);
+	if (ep->umidi->usb_protocol_ops->finish_out_endpoint)
+		ep->umidi->usb_protocol_ops->finish_out_endpoint(ep);
+	ep->active_urbs = 0;
+	if (ep->drain_urbs) {
+		ep->drain_urbs = 0;
+		wake_up(&ep->drain_wait);
+	}
+
 	for (i = 0; i < OUTPUT_URBS; ++i)
 		if (ep->urbs[i].urb) {
 			free_urb_and_buffer(ep->umidi, ep->urbs[i].urb,
@@ -1469,7 +1485,7 @@ static void snd_usbmidi_free(struct snd_usb_midi *umidi)
 void snd_usbmidi_disconnect(struct list_head *p)
 {
 	struct snd_usb_midi *umidi;
-	unsigned int i, j;
+	unsigned int i;
 
 	umidi = list_entry(p, struct snd_usb_midi, list);
 	/*
@@ -1487,22 +1503,6 @@ void snd_usbmidi_disconnect(struct list_head *p)
 
 	for (i = 0; i < MIDI_MAX_ENDPOINTS; ++i) {
 		struct snd_usb_midi_endpoint *ep = &umidi->endpoints[i];
-		if (ep->out)
-			cancel_work_sync(&ep->out->work);
-		if (ep->out) {
-			for (j = 0; j < OUTPUT_URBS; ++j)
-				usb_kill_urb(ep->out->urbs[j].urb);
-			if (umidi->usb_protocol_ops->finish_out_endpoint)
-				umidi->usb_protocol_ops->finish_out_endpoint(ep->out);
-			ep->out->active_urbs = 0;
-			if (ep->out->drain_urbs) {
-				ep->out->drain_urbs = 0;
-				wake_up(&ep->out->drain_wait);
-			}
-		}
-		if (ep->in)
-			for (j = 0; j < INPUT_URBS; ++j)
-				usb_kill_urb(ep->in->urbs[j]);
 		/* free endpoints here; later call can result in Oops */
 		if (ep->out)
 			snd_usbmidi_out_endpoint_clear(ep->out);
-- 
2.31.1

