* [PATCH] Fix potential NULL pointer dereference in echoaudio midi. @ 2006-10-31 21:21 Jesper Juhl 2006-10-31 22:13 ` David Rientjes 0 siblings, 1 reply; 6+ messages in thread From: Jesper Juhl @ 2006-10-31 21:21 UTC (permalink / raw) To: linux-kernel; +Cc: Giuliano Pochini, Takashi Iwai, Jesper Juhl In sound/pci/echoaudio/midi.c::snd_echo_midi_output_write(), there's a risk of dereferencing a NULL 'chip->midi_out'. This patch contains the obvious fix as also used a bit higher up in the same function. Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com> --- diff --git a/sound/pci/echoaudio/midi.c b/sound/pci/echoaudio/midi.c index e31f0f1..69ef9f0 100644 --- a/sound/pci/echoaudio/midi.c +++ b/sound/pci/echoaudio/midi.c @@ -236,7 +236,8 @@ static void snd_echo_midi_output_write(u } /* We restart the timer only if there is some data left to send */ - if (!snd_rawmidi_transmit_empty(chip->midi_out) && chip->tinuse) { + if (chip->midi_out && !snd_rawmidi_transmit_empty(chip->midi_out) && + chip->tinuse) { /* The timer will expire slightly after the data has been sent */ time = (sent << 3) / 25 + 1; /* 8/25=0.32ms to send a byte */ ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix potential NULL pointer dereference in echoaudio midi. 2006-10-31 21:21 [PATCH] Fix potential NULL pointer dereference in echoaudio midi Jesper Juhl @ 2006-10-31 22:13 ` David Rientjes 2006-10-31 22:26 ` Jesper Juhl 0 siblings, 1 reply; 6+ messages in thread From: David Rientjes @ 2006-10-31 22:13 UTC (permalink / raw) To: Jesper Juhl; +Cc: linux-kernel, Giuliano Pochini, Takashi Iwai On Tue, 31 Oct 2006, Jesper Juhl wrote: > In sound/pci/echoaudio/midi.c::snd_echo_midi_output_write(), there's a risk > of dereferencing a NULL 'chip->midi_out'. > This patch contains the obvious fix as also used a bit higher up in the > same function. > How about just adding an early test: if (!chip->midi_out) goto out; and adding a label for out before the chip->lock unlock? We still need to clear chip->midi_full so we still require the spinlock, but there's no reason we should be testing chip->midi_out multiple times since the remaining code path in its entirety depends on it. David ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix potential NULL pointer dereference in echoaudio midi. 2006-10-31 22:13 ` David Rientjes @ 2006-10-31 22:26 ` Jesper Juhl 2006-11-02 20:16 ` Giuliano Pochini 0 siblings, 1 reply; 6+ messages in thread From: Jesper Juhl @ 2006-10-31 22:26 UTC (permalink / raw) To: David Rientjes; +Cc: linux-kernel, Giuliano Pochini, Takashi Iwai, Jesper Juhl On Tuesday 31 October 2006 23:13, David Rientjes wrote: > On Tue, 31 Oct 2006, Jesper Juhl wrote: > > > In sound/pci/echoaudio/midi.c::snd_echo_midi_output_write(), there's a risk > > of dereferencing a NULL 'chip->midi_out'. > > This patch contains the obvious fix as also used a bit higher up in the > > same function. > > > > How about just adding an early test: > if (!chip->midi_out) > goto out; > > and adding a label for out before the chip->lock unlock? We still need to > clear chip->midi_full so we still require the spinlock, but there's no > reason we should be testing chip->midi_out multiple times since the > remaining code path in its entirety depends on it. > Sure, that's an alternative solution. Probably a superiour one since, as you say, we'll then only be testing 'chip->midi_out' once. Here's a patch that makes that change instead. I'll leave it up to the powers-that-be to pick the one they like best :) Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com> --- diff --git a/sound/pci/echoaudio/midi.c b/sound/pci/echoaudio/midi.c index e31f0f1..0385b4e 100644 --- a/sound/pci/echoaudio/midi.c +++ b/sound/pci/echoaudio/midi.c @@ -213,7 +213,9 @@ static void snd_echo_midi_output_write(u sent = bytes = 0; spin_lock_irqsave(&chip->lock, flags); chip->midi_full = 0; - if (chip->midi_out && !snd_rawmidi_transmit_empty(chip->midi_out)) { + if (!chip->midi_out) + goto out; + if (!snd_rawmidi_transmit_empty(chip->midi_out)) { bytes = snd_rawmidi_transmit_peek(chip->midi_out, buf, MIDI_OUT_BUFFER_SIZE - 1); DE_MID(("Try to send %d bytes...\n", bytes)); @@ -243,6 +245,7 @@ static void snd_echo_midi_output_write(u mod_timer(&chip->timer, jiffies + (time * HZ + 999) / 1000); DE_MID(("Timer armed(%d)\n", ((time * HZ + 999) / 1000))); } + out: spin_unlock_irqrestore(&chip->lock, flags); } ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix potential NULL pointer dereference in echoaudio midi. 2006-10-31 22:26 ` Jesper Juhl @ 2006-11-02 20:16 ` Giuliano Pochini 2006-11-02 20:22 ` Jesper Juhl 0 siblings, 1 reply; 6+ messages in thread From: Giuliano Pochini @ 2006-11-02 20:16 UTC (permalink / raw) To: Jesper Juhl; +Cc: rientjes, linux-kernel, tiwai, jesper.juhl On Tue, 31 Oct 2006 23:26:31 +0100 Jesper Juhl <jesper.juhl@gmail.com> wrote: > On Tuesday 31 October 2006 23:13, David Rientjes wrote: > > On Tue, 31 Oct 2006, Jesper Juhl wrote: > > > > > In sound/pci/echoaudio/midi.c::snd_echo_midi_output_write(), there's a risk > > > of dereferencing a NULL 'chip->midi_out'. > > > This patch contains the obvious fix as also used a bit higher up in the > > > same function. > > > > > > > How about just adding an early test: > > if (!chip->midi_out) > > goto out; The point of that check is to make sure is doesn't access chip->midi_out when (surprise!) it is NULL. This can only happen in the rare (possible?) case snd_echo_midi_output_close() is called while the timer handler is running. I have another proposal which IMHO is smp-safer that just moving the check. In that case we should also put a spinlock around the chip->midi_out=0 in the snd_echo_midi_output_close() callback. Signed-off-by: Giuliano Pochini <pochini@shiny.it> --- alsa-kernel/pci/echoaudio/midi.c__orig 2006-11-02 20:39:45.000000000 +0100 +++ alsa-kernel/pci/echoaudio/midi.c 2006-11-02 20:44:22.000000000 +0100 @@ -213,7 +213,7 @@ static void snd_echo_midi_output_write(u sent = bytes = 0; spin_lock_irqsave(&chip->lock, flags); chip->midi_full = 0; - if (chip->midi_out && !snd_rawmidi_transmit_empty(chip->midi_out)) { + if (!snd_rawmidi_transmit_empty(chip->midi_out)) { bytes = snd_rawmidi_transmit_peek(chip->midi_out, buf, MIDI_OUT_BUFFER_SIZE - 1); DE_MID(("Try to send %d bytes...\n", bytes)); @@ -264,9 +264,11 @@ static void snd_echo_midi_output_trigger } } else { if (chip->tinuse) { - del_timer(&chip->timer); chip->tinuse = 0; + spin_unlock_irq(&chip->lock); + del_timer_sync(&chip->timer); DE_MID(("Timer removed\n")); + return; } } spin_unlock_irq(&chip->lock); -- Giuliano. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix potential NULL pointer dereference in echoaudio midi. 2006-11-02 20:16 ` Giuliano Pochini @ 2006-11-02 20:22 ` Jesper Juhl 2006-11-06 10:50 ` Takashi Iwai 0 siblings, 1 reply; 6+ messages in thread From: Jesper Juhl @ 2006-11-02 20:22 UTC (permalink / raw) To: Giuliano Pochini; +Cc: rientjes, linux-kernel, tiwai On 02/11/06, Giuliano Pochini <pochini@shiny.it> wrote: > On Tue, 31 Oct 2006 23:26:31 +0100 > Jesper Juhl <jesper.juhl@gmail.com> wrote: > > > On Tuesday 31 October 2006 23:13, David Rientjes wrote: > > > On Tue, 31 Oct 2006, Jesper Juhl wrote: > > > > > > > In sound/pci/echoaudio/midi.c::snd_echo_midi_output_write(), there's a risk > > > > of dereferencing a NULL 'chip->midi_out'. > > > > This patch contains the obvious fix as also used a bit higher up in the > > > > same function. > > > > > > > > > > How about just adding an early test: > > > if (!chip->midi_out) > > > goto out; > > > The point of that check is to make sure is doesn't access chip->midi_out > when (surprise!) it is NULL. This can only happen in the rare (possible?) > case snd_echo_midi_output_close() is called while the timer handler is > running. I have another proposal which IMHO is smp-safer that just moving > the check. In that case we should also put a spinlock around the > chip->midi_out=0 in the snd_echo_midi_output_close() callback. > > > Signed-off-by: Giuliano Pochini <pochini@shiny.it> > > --- alsa-kernel/pci/echoaudio/midi.c__orig 2006-11-02 20:39:45.000000000 +0100 > +++ alsa-kernel/pci/echoaudio/midi.c 2006-11-02 20:44:22.000000000 +0100 > @@ -213,7 +213,7 @@ static void snd_echo_midi_output_write(u > sent = bytes = 0; > spin_lock_irqsave(&chip->lock, flags); > chip->midi_full = 0; > - if (chip->midi_out && !snd_rawmidi_transmit_empty(chip->midi_out)) { > + if (!snd_rawmidi_transmit_empty(chip->midi_out)) { > bytes = snd_rawmidi_transmit_peek(chip->midi_out, buf, > MIDI_OUT_BUFFER_SIZE - 1); > DE_MID(("Try to send %d bytes...\n", bytes)); > @@ -264,9 +264,11 @@ static void snd_echo_midi_output_trigger > } > } else { > if (chip->tinuse) { > - del_timer(&chip->timer); > chip->tinuse = 0; > + spin_unlock_irq(&chip->lock); > + del_timer_sync(&chip->timer); > DE_MID(("Timer removed\n")); > + return; > } > } > spin_unlock_irq(&chip->lock); > Fine by me. Let's just get one of the versions pushed into -mm or mainline :) -- Jesper Juhl <jesper.juhl@gmail.com> Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html Plain text mails only, please http://www.expita.com/nomime.html ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix potential NULL pointer dereference in echoaudio midi. 2006-11-02 20:22 ` Jesper Juhl @ 2006-11-06 10:50 ` Takashi Iwai 0 siblings, 0 replies; 6+ messages in thread From: Takashi Iwai @ 2006-11-06 10:50 UTC (permalink / raw) To: Jesper Juhl; +Cc: Giuliano Pochini, rientjes, linux-kernel At Thu, 2 Nov 2006 21:22:43 +0100, Jesper Juhl wrote: > > On 02/11/06, Giuliano Pochini <pochini@shiny.it> wrote: > > On Tue, 31 Oct 2006 23:26:31 +0100 > > Jesper Juhl <jesper.juhl@gmail.com> wrote: > > > > > On Tuesday 31 October 2006 23:13, David Rientjes wrote: > > > > On Tue, 31 Oct 2006, Jesper Juhl wrote: > > > > > > > > > In sound/pci/echoaudio/midi.c::snd_echo_midi_output_write(), there's a risk > > > > > of dereferencing a NULL 'chip->midi_out'. > > > > > This patch contains the obvious fix as also used a bit higher up in the > > > > > same function. > > > > > > > > > > > > > How about just adding an early test: > > > > if (!chip->midi_out) > > > > goto out; > > > > > > The point of that check is to make sure is doesn't access chip->midi_out > > when (surprise!) it is NULL. This can only happen in the rare (possible?) > > case snd_echo_midi_output_close() is called while the timer handler is > > running. I have another proposal which IMHO is smp-safer that just moving > > the check. In that case we should also put a spinlock around the > > chip->midi_out=0 in the snd_echo_midi_output_close() callback. > > > > > > Signed-off-by: Giuliano Pochini <pochini@shiny.it> > > > > --- alsa-kernel/pci/echoaudio/midi.c__orig 2006-11-02 20:39:45.000000000 +0100 > > +++ alsa-kernel/pci/echoaudio/midi.c 2006-11-02 20:44:22.000000000 +0100 > > @@ -213,7 +213,7 @@ static void snd_echo_midi_output_write(u > > sent = bytes = 0; > > spin_lock_irqsave(&chip->lock, flags); > > chip->midi_full = 0; > > - if (chip->midi_out && !snd_rawmidi_transmit_empty(chip->midi_out)) { > > + if (!snd_rawmidi_transmit_empty(chip->midi_out)) { > > bytes = snd_rawmidi_transmit_peek(chip->midi_out, buf, > > MIDI_OUT_BUFFER_SIZE - 1); > > DE_MID(("Try to send %d bytes...\n", bytes)); > > @@ -264,9 +264,11 @@ static void snd_echo_midi_output_trigger > > } > > } else { > > if (chip->tinuse) { > > - del_timer(&chip->timer); > > chip->tinuse = 0; > > + spin_unlock_irq(&chip->lock); > > + del_timer_sync(&chip->timer); > > DE_MID(("Timer removed\n")); > > + return; > > } > > } > > spin_unlock_irq(&chip->lock); > > > > Fine by me. > Let's just get one of the versions pushed into -mm or mainline :) I applied it to ALSA tree mm branch. Thanks. Takashi ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2006-11-06 10:50 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2006-10-31 21:21 [PATCH] Fix potential NULL pointer dereference in echoaudio midi Jesper Juhl 2006-10-31 22:13 ` David Rientjes 2006-10-31 22:26 ` Jesper Juhl 2006-11-02 20:16 ` Giuliano Pochini 2006-11-02 20:22 ` Jesper Juhl 2006-11-06 10:50 ` Takashi Iwai
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox