linux-kernel - Re: [PATCH 1/1] ALSA: usb-audio: Avoid NULL dereference in snd_usbmidi_do

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87plbhn16a.wl-tiwai@suse.de>
Date: Tue, 23 Sep 2025 08:16:45 +0200
From: Takashi Iwai <tiwai@...e.de>
To: Alan Stern <stern@...land.harvard.edu>
Cc: Brahmajit Das <listout@...tout.xyz>,
	syzbot+f02665daa2abeef4a947@...kaller.appspotmail.com,
	clemens@...isch.de,
	linux-kernel@...r.kernel.org,
	linux-sound@...r.kernel.org,
	linux-usb@...r.kernel.org,
	perex@...ex.cz,
	syzkaller-bugs@...glegroups.com,
	tiwai@...e.com
Subject: Re: [PATCH 1/1] ALSA: usb-audio: Avoid NULL dereference in snd_usbmidi_do_output()

On Tue, 23 Sep 2025 04:40:13 +0200,
Alan Stern wrote:
> 
> On Tue, Sep 23, 2025 at 04:47:20AM +0530, Brahmajit Das wrote:
> > Syzkaller reported a general protection fault in snd_usbmidi_do_output(),
> > caused by dereferencing a NULL URB pointer when accessing
> > ep->urbs[urb_index].urb.
> > 
> > This can happen in rare race conditions where the URB was not initialized
> > or was already freed (e.g. during disconnect or after errors), and the
> > output timer or other path tries to reuse it.
> > 
> > Fix this by checking if the URB is NULL before accessing it, and skipping
> > the current slot if it is.
> > 
> > Reported-by: syzbot+f02665daa2abeef4a947@...kaller.appspotmail.com
> > Link: https://syzkaller.appspot.com/bug?extid=f02665daa2abeef4a947
> > 
> > Signed-off-by: Brahmajit Das <listout@...tout.xyz>
> > ---
> >  sound/usb/midi.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/sound/usb/midi.c b/sound/usb/midi.c
> > index acb3bf92857c..7919a39decb4 100644
> > --- a/sound/usb/midi.c
> > +++ b/sound/usb/midi.c
> > @@ -307,6 +307,10 @@ static void snd_usbmidi_do_output(struct snd_usb_midi_out_endpoint *ep)
> >  	for (;;) {
> >  		if (!(ep->active_urbs & (1 << urb_index))) {
> >  			urb = ep->urbs[urb_index].urb;
> > +			if (!urb) {
> > +				// Skip this urb
> > +				goto next_urb;
> > +			}
> 
> What prevents the URB from being freed right here?  If this happens, 
> the code below would access memory that was deallocated.
> 
> To prevent races, you have to use some sort of lock or other 
> synchronization mechanism.  A simple test won't work.

Yes.  The timer instance itself should have been killed before
releasing the resources.

I guess the patch below could cover it, but since I'm traveling, I
can't check it for now.  Will check later once after I'm back.


thanks,

Takashi

--- a/sound/usb/midi.c
+++ b/sound/usb/midi.c
@@ -1511,6 +1511,7 @@ static void snd_usbmidi_free(struct snd_usb_midi *umidi)
 {
 	int i;
 
+	timer_shutdown_sync(&umidi->error_timer);
 	for (i = 0; i < MIDI_MAX_ENDPOINTS; ++i) {
 		struct snd_usb_midi_endpoint *ep = &umidi->endpoints[i];
 		if (ep->out)
@@ -1519,7 +1520,6 @@ static void snd_usbmidi_free(struct snd_usb_midi *umidi)
 			snd_usbmidi_in_endpoint_delete(ep->in);
 	}
 	mutex_destroy(&umidi->mutex);
-	timer_shutdown_sync(&umidi->error_timer);
 	kfree(umidi);
 }