Re: sound: out-of-bounds write in snd_rawmidi_kernel_write1

[Dmitry Vyukov]

On Wed, Feb 3, 2016 at 1:02 PM, Takashi Iwai <tiwai@xxxxxxx> wrote:
> On Wed, 03 Feb 2016 12:39:31 +0100,
> Takashi Iwai wrote:
>>
>> On Wed, 03 Feb 2016 10:41:14 +0100,
>> Takashi Iwai wrote:
>> >
>> > On Wed, 03 Feb 2016 10:35:14 +0100,
>> > Takashi Iwai wrote:
>> > >
>> > > On Wed, 03 Feb 2016 09:57:50 +0100,
>> > > Dmitry Vyukov wrote:
>> > > >
>> > > > Hello,
>> > > >
>> > > > The following program triggers an out-of-bounds write in
>> > > > snd_rawmidi_kernel_write1 (run in parallel loop). It seems to try to
>> > > > copy -1 bytes (aka 4GB) from user space into kernel smashing all on
>> > > > its way.
>> > >
>> > > What card is /dev/midi3?  Please check /proc/asound/cards.
>> > > Is it MTPAV?
>> >
>> > In anyway the patch below should paper over it.  But it's still
>> > strange that it gets a negative value there.  Could you put
>> >
>> >    WARN_ON(count1 < 0)
>> >
>> > before the newly added check?
>> >
>> > I tried it locally with virmidi but it didn't appear, so far.  Maybe
>> > my setup is too slow and has fewer CPUs than yours.
>>
>> Scratch my previous patch, I could reproduce the issue on a faster
>> machine in my office now :)  Will work on it.
>
> This turned out to be a race in updates of runtime->appl_ptr & co.
> We do temporary spin unlock and relock while copying the user-space
> data, and then update these values.  Meanwhile these values are
> referred as the position to copy, and the concurrent accesses may lead
> to the negative value.
>
> Below is a quick fix for that, just updating these before the
> temporary unlock.
>
> The patch also fixes the read size where it has more race problems...
>
> It seems working on my machine.  Let me know if this works for you,
> too.  Then I'll cook up the official patch.


Yes, it fixes the crash for me.  Thanks!


> ---
> diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
> index 26ca02248885..795437b10082 100644
> --- a/sound/core/rawmidi.c
> +++ b/sound/core/rawmidi.c
> @@ -942,31 +942,36 @@ static long snd_rawmidi_kernel_read1(struct snd_rawmidi_substream *substream,
>         unsigned long flags;
>         long result = 0, count1;
>         struct snd_rawmidi_runtime *runtime = substream->runtime;
> +       unsigned long appl_ptr;
>
> +       spin_lock_irqsave(&runtime->lock, flags);
>         while (count > 0 && runtime->avail) {
>                 count1 = runtime->buffer_size - runtime->appl_ptr;
>                 if (count1 > count)
>                         count1 = count;
> -               spin_lock_irqsave(&runtime->lock, flags);
>                 if (count1 > (int)runtime->avail)
>                         count1 = runtime->avail;
> +
> +               /* update runtime->appl_ptr before unlocking for userbuf */
> +               appl_ptr = runtime->appl_ptr;
> +               runtime->appl_ptr += count1;
> +               runtime->appl_ptr %= runtime->buffer_size;
> +               runtime->avail -= count1;
> +
>                 if (kernelbuf)
> -                       memcpy(kernelbuf + result, runtime->buffer + runtime->appl_ptr, count1);
> +                       memcpy(kernelbuf + result, runtime->buffer + appl_ptr, count1);
>                 if (userbuf) {
>                         spin_unlock_irqrestore(&runtime->lock, flags);
>                         if (copy_to_user(userbuf + result,
> -                                        runtime->buffer + runtime->appl_ptr, count1)) {
> +                                        runtime->buffer + appl_ptr, count1)) {
>                                 return result > 0 ? result : -EFAULT;
>                         }
>                         spin_lock_irqsave(&runtime->lock, flags);
>                 }
> -               runtime->appl_ptr += count1;
> -               runtime->appl_ptr %= runtime->buffer_size;
> -               runtime->avail -= count1;
> -               spin_unlock_irqrestore(&runtime->lock, flags);
>                 result += count1;
>                 count -= count1;
>         }
> +       spin_unlock_irqrestore(&runtime->lock, flags);
>         return result;
>  }
>
> @@ -1223,6 +1228,7 @@ static long snd_rawmidi_kernel_write1(struct snd_rawmidi_substream *substream,
>         unsigned long flags;
>         long count1, result;
>         struct snd_rawmidi_runtime *runtime = substream->runtime;
> +       unsigned long appl_ptr;
>
>         if (!kernelbuf && !userbuf)
>                 return -EINVAL;
> @@ -1243,12 +1249,19 @@ static long snd_rawmidi_kernel_write1(struct snd_rawmidi_substream *substream,
>                         count1 = count;
>                 if (count1 > (long)runtime->avail)
>                         count1 = runtime->avail;
> +
> +               /* update runtime->appl_ptr before unlocking for userbuf */
> +               appl_ptr = runtime->appl_ptr;
> +               runtime->appl_ptr += count1;
> +               runtime->appl_ptr %= runtime->buffer_size;
> +               runtime->avail -= count1;
> +
>                 if (kernelbuf)
> -                       memcpy(runtime->buffer + runtime->appl_ptr,
> +                       memcpy(runtime->buffer + appl_ptr,
>                                kernelbuf + result, count1);
>                 else if (userbuf) {
>                         spin_unlock_irqrestore(&runtime->lock, flags);
> -                       if (copy_from_user(runtime->buffer + runtime->appl_ptr,
> +                       if (copy_from_user(runtime->buffer + appl_ptr,
>                                            userbuf + result, count1)) {
>                                 spin_lock_irqsave(&runtime->lock, flags);
>                                 result = result > 0 ? result : -EFAULT;
> @@ -1256,9 +1269,6 @@ static long snd_rawmidi_kernel_write1(struct snd_rawmidi_substream *substream,
>                         }
>                         spin_lock_irqsave(&runtime->lock, flags);
>                 }
> -               runtime->appl_ptr += count1;
> -               runtime->appl_ptr %= runtime->buffer_size;
> -               runtime->avail -= count1;
>                 result += count1;
>                 count -= count1;
>         }