Re: [PATCH] Fix potential NULL pointer dereference in echoaudio midi.

[Takashi Iwai <tiwai@suse.de>]

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