[alsa-devel] [PATCH v2 1/2] ALSA: fireworks: accessing to user space outside spinlock

[Takashi Iwai]

On Tue, 30 Aug 2016 01:01:34 +0200,
Takashi Sakamoto wrote:
> 
> In hwdep interface of fireworks driver, accessing to user space is in a
> critical section with disabled local interrupt. Depending on architecture,
> accessing to user space can cause page fault exception. Then local
> processor stores machine status and handles the synchronous event. A
> handler corresponding to the event can call task scheduler to wait for
> preparing pages. In a case of usage of single core processor, the state to
> disable local interrupt is worse because it don't handle usual interrupts
> from hardware.
> 
> This commit fixes this bug, performing the accessing outside spinlock.
> 
> Reported-by: Vaishali Thakkar <vaishali.thakkar at oracle.com>
> Cc: stable at vger.kernel.org
> Fixes: 555e8a8f7f14('ALSA: fireworks: Add command/response functionality into hwdep interface')
> Signed-off-by: Takashi Sakamoto <o-takashi at sakamocchi.jp>
> ---
>  sound/firewire/fireworks/fireworks_hwdep.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/sound/firewire/fireworks/fireworks_hwdep.c b/sound/firewire/fireworks/fireworks_hwdep.c
> index 33df865..8ed4b29 100644
> --- a/sound/firewire/fireworks/fireworks_hwdep.c
> +++ b/sound/firewire/fireworks/fireworks_hwdep.c
> @@ -38,6 +38,7 @@ hwdep_read_resp_buf(struct snd_efw *efw, char __user *buf, long remained,
>  	buf += sizeof(type);
>  
>  	/* write into buffer as many responses as possible */
> +	spin_lock_irq(&efw->lock);
>  	while (efw->resp_queues > 0) {
>  		t = (struct snd_efw_transaction *)(efw->pull_ptr);
>  		length = be32_to_cpu(t->length) * sizeof(__be32);
> @@ -52,9 +53,13 @@ hwdep_read_resp_buf(struct snd_efw *efw, char __user *buf, long remained,
>  				(unsigned int)(efw->pull_ptr - efw->resp_buf);
>  			till_end = min_t(unsigned int, length, till_end);
>  
> +			spin_unlock_irq(&efw->lock);
> +
>  			if (copy_to_user(buf, efw->pull_ptr, till_end))
>  				return -EFAULT;
>  
> +			spin_lock_irq(&efw->lock);
> +
>  			efw->pull_ptr += till_end;

Strictly speaking, this is still racy.  efw->pull_ptr is accessed
outside the spinlock, and it can be modified while copy_to_user() is
being processed.


Takashi

>  			if (efw->pull_ptr >= efw->resp_buf +
>  					     snd_efw_resp_buf_size)
> @@ -69,6 +74,8 @@ hwdep_read_resp_buf(struct snd_efw *efw, char __user *buf, long remained,
>  		efw->resp_queues--;
>  	}
>  
> +	spin_unlock_irq(&efw->lock);
> +
>  	return count;
>  }
>  
> @@ -76,14 +83,17 @@ static long
>  hwdep_read_locked(struct snd_efw *efw, char __user *buf, long count,
>  		  loff_t *offset)
>  {
> -	union snd_firewire_event event;
> +	union snd_firewire_event event = {
> +		.lock_status.type = SNDRV_FIREWIRE_EVENT_LOCK_STATUS,
> +	};
>  
> -	memset(&event, 0, sizeof(event));
> +	spin_lock_irq(&efw->lock);
>  
> -	event.lock_status.type = SNDRV_FIREWIRE_EVENT_LOCK_STATUS;
>  	event.lock_status.status = (efw->dev_lock_count > 0);
>  	efw->dev_lock_changed = false;
>  
> +	spin_unlock_irq(&efw->lock);
> +
>  	count = min_t(long, count, sizeof(event.lock_status));
>  
>  	if (copy_to_user(buf, &event, count))
> @@ -111,13 +121,14 @@ hwdep_read(struct snd_hwdep *hwdep, char __user *buf, long count,
>  		spin_lock_irq(&efw->lock);
>  	}
>  
> +	/* Loosely release the lock here, but still safe. */
> +	spin_unlock_irq(&efw->lock);
> +
>  	if (efw->dev_lock_changed)
>  		count = hwdep_read_locked(efw, buf, count, offset);
>  	else if (efw->resp_queues > 0)
>  		count = hwdep_read_resp_buf(efw, buf, count, offset);
>  
> -	spin_unlock_irq(&efw->lock);
> -
>  	return count;
>  }
>  
> -- 
> 2.7.4
>