[SRU,Trusty,Bionic,1/1] ALSA: rawmidi: Change resized buffers atomically

From: Takashi Iwai <tiwai@suse.de>

CVE-2018-10902

The SNDRV_RAWMIDI_IOCTL_PARAMS ioctl may resize the buffers and the
current code is racy.  For example, the sequencer client may write to
buffer while it being resized.

As a simple workaround, let's switch to the resized buffer inside the
stream runtime lock.

Change-Id: I780f33f62670b4ad93cf92513aa4b87ff41bc63e
Reported-by: syzbot+52f83f0ea8df16932f7f@syzkaller.appspotmail.com
(cherry picked from commit 39675f7a7c7e7702f7d5341f1e0d01db746543a0)
Signed-off-by: Khalid Elmously <khalid.elmously@canonical.com>

Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/rawmidi.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

On 2018-11-23 02:28:35, Khalid Elmously wrote:
> From: Takashi Iwai <tiwai@suse.de>
> 
> CVE-2018-10902
> 
> The SNDRV_RAWMIDI_IOCTL_PARAMS ioctl may resize the buffers and the
> current code is racy.  For example, the sequencer client may write to
> buffer while it being resized.
> 
> As a simple workaround, let's switch to the resized buffer inside the
> stream runtime lock.
> 
> Change-Id: I780f33f62670b4ad93cf92513aa4b87ff41bc63e
> Reported-by: syzbot+52f83f0ea8df16932f7f@syzkaller.appspotmail.com
> (cherry picked from commit 39675f7a7c7e7702f7d5341f1e0d01db746543a0)
> Signed-off-by: Khalid Elmously <khalid.elmously@canonical.com>
> 
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

Acked-by: Tyler Hicks <tyhicks@canonical.com>

Thanks!

Tyler

On 11/23/18 8:28 AM, Khalid Elmously wrote:
> From: Takashi Iwai <tiwai@suse.de>
>
> CVE-2018-10902
>
> The SNDRV_RAWMIDI_IOCTL_PARAMS ioctl may resize the buffers and the
> current code is racy.  For example, the sequencer client may write to
> buffer while it being resized.
>
> As a simple workaround, let's switch to the resized buffer inside the
> stream runtime lock.
>
> Change-Id: I780f33f62670b4ad93cf92513aa4b87ff41bc63e
Is the above line necessary? It doesn't exist on the original patch.
> Reported-by: syzbot+52f83f0ea8df16932f7f@syzkaller.appspotmail.com
> (cherry picked from commit 39675f7a7c7e7702f7d5341f1e0d01db746543a0)
> Signed-off-by: Khalid Elmously <khalid.elmously@canonical.com>
>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

Please always add the additional S-O-B and provenance information below
the original patch, the original commit message should be kept intact
(apart from adding a Buglink or CVE reference as the first line).

Anyway this can be fixed when applying it.

Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>

> ---
>  sound/core/rawmidi.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
> index a15f63fde842..ee6e80a40774 100644
> --- a/sound/core/rawmidi.c
> +++ b/sound/core/rawmidi.c
> @@ -627,7 +627,7 @@ static int snd_rawmidi_info_select_user(struct snd_card *card,
>  int snd_rawmidi_output_params(struct snd_rawmidi_substream *substream,
>  			      struct snd_rawmidi_params * params)
>  {
> -	char *newbuf;
> +	char *newbuf, *oldbuf;
>  	struct snd_rawmidi_runtime *runtime = substream->runtime;
>  	
>  	if (substream->append && substream->use_count > 1)
> @@ -640,13 +640,17 @@ int snd_rawmidi_output_params(struct snd_rawmidi_substream *substream,
>  		return -EINVAL;
>  	}
>  	if (params->buffer_size != runtime->buffer_size) {
> -		newbuf = krealloc(runtime->buffer, params->buffer_size,
> -				  GFP_KERNEL);
> +		newbuf = kmalloc(params->buffer_size, GFP_KERNEL);
>  		if (!newbuf)
>  			return -ENOMEM;
> +		spin_lock_irq(&runtime->lock);
> +		oldbuf = runtime->buffer;
>  		runtime->buffer = newbuf;
>  		runtime->buffer_size = params->buffer_size;
>  		runtime->avail = runtime->buffer_size;
> +		runtime->appl_ptr = runtime->hw_ptr = 0;
> +		spin_unlock_irq(&runtime->lock);
> +		kfree(oldbuf);
>  	}
>  	runtime->avail_min = params->avail_min;
>  	substream->active_sensing = !params->no_active_sensing;
> @@ -657,7 +661,7 @@ EXPORT_SYMBOL(snd_rawmidi_output_params);
>  int snd_rawmidi_input_params(struct snd_rawmidi_substream *substream,
>  			     struct snd_rawmidi_params * params)
>  {
> -	char *newbuf;
> +	char *newbuf, *oldbuf;
>  	struct snd_rawmidi_runtime *runtime = substream->runtime;
>  
>  	snd_rawmidi_drain_input(substream);
> @@ -668,12 +672,16 @@ int snd_rawmidi_input_params(struct snd_rawmidi_substream *substream,
>  		return -EINVAL;
>  	}
>  	if (params->buffer_size != runtime->buffer_size) {
> -		newbuf = krealloc(runtime->buffer, params->buffer_size,
> -				  GFP_KERNEL);
> +		newbuf = kmalloc(params->buffer_size, GFP_KERNEL);
>  		if (!newbuf)
>  			return -ENOMEM;
> +		spin_lock_irq(&runtime->lock);
> +		oldbuf = runtime->buffer;
>  		runtime->buffer = newbuf;
>  		runtime->buffer_size = params->buffer_size;
> +		runtime->appl_ptr = runtime->hw_ptr = 0;
> +		spin_unlock_irq(&runtime->lock);
> +		kfree(oldbuf);
>  	}
>  	runtime->avail_min = params->avail_min;
>  	return 0;