On Tuesday 30 August 2016 10:49 AM, Takashi Iwai wrote:
> 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.
Should disabling pagefaults while interacting with user space work here?
>
> 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
>>
--
Vaishali
--
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