From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from aserp1040.oracle.com ([141.146.126.69]:32688 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757784AbcH3GHn (ORCPT ); Tue, 30 Aug 2016 02:07:43 -0400 Subject: Re: [PATCH v2 1/2] ALSA: fireworks: accessing to user space outside spinlock To: Takashi Iwai , Takashi Sakamoto References: <1472511695-29316-1-git-send-email-o-takashi@sakamocchi.jp> <1472511695-29316-2-git-send-email-o-takashi@sakamocchi.jp> Cc: clemens@ladisch.de, alsa-devel@alsa-project.org, ffado-devel@lists.sf.net, stable@vger.kernel.org From: Vaishali Thakkar Message-ID: <57C52288.4010502@oracle.com> Date: Tue, 30 Aug 2016 11:37:04 +0530 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: stable-owner@vger.kernel.org List-ID: 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 >> Cc: stable@vger.kernel.org >> Fixes: 555e8a8f7f14('ALSA: fireworks: Add command/response functionality into hwdep interface') >> Signed-off-by: Takashi Sakamoto >> --- >> 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