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 <[email protected]>
---
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 */
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
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 <[email protected]>
---
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);
}
On Tue, 31 Oct 2006 23:26:31 +0100
Jesper Juhl <[email protected]> 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 <[email protected]>
--- 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.
On 02/11/06, Giuliano Pochini <[email protected]> wrote:
> On Tue, 31 Oct 2006 23:26:31 +0100
> Jesper Juhl <[email protected]> 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 <[email protected]>
>
> --- 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 <[email protected]>
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
At Thu, 2 Nov 2006 21:22:43 +0100,
Jesper Juhl wrote:
>
> On 02/11/06, Giuliano Pochini <[email protected]> wrote:
> > On Tue, 31 Oct 2006 23:26:31 +0100
> > Jesper Juhl <[email protected]> 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 <[email protected]>
> >
> > --- 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