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@xxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 555e8a8f7f14('ALSA: fireworks: Add command/response functionality into hwdep interface')
> Signed-off-by: Takashi Sakamoto <o-takashi@xxxxxxxxxxxxx>
> ---
> 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
>
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html