All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ALSA: fireface: fix locking bug in ff400_copy_msg_to_user()
@ 2023-01-17 14:17 ` Dan Carpenter
  0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2023-01-17 14:17 UTC (permalink / raw)
  To: Takashi Sakamoto
  Cc: Clemens Ladisch, Takashi Sakamoto, Jaroslav Kysela, Takashi Iwai,
	alsa-devel, kernel-janitors

The ff400_copy_msg_to_user() function drops the spin lock to call
copy_to_user().  However, if the copy_to_user() fails, then it must
take the lock again before returning.  Failure to take the lock leads
to a double unlock in the caller, hwdep_read().

Fixes: acdebd8b4c0c ("ALSA: fireface: implement message parser for Fireface 400")
Signed-off-by: Dan Carpenter <error27@gmail.com>
---
 sound/firewire/fireface/ff-protocol-former.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/sound/firewire/fireface/ff-protocol-former.c b/sound/firewire/fireface/ff-protocol-former.c
index f58008762fe6..fa41de978756 100644
--- a/sound/firewire/fireface/ff-protocol-former.c
+++ b/sound/firewire/fireface/ff-protocol-former.c
@@ -680,28 +680,30 @@ static long ff400_copy_msg_to_user(struct snd_ff *ff, char __user *buf, long cou
 	struct ff400_msg_parser *parser = ff->msg_parser;
 	u32 type = SNDRV_FIREWIRE_EVENT_FF400_MESSAGE;
 	long consumed = 0;
+	int ret = 0;
 
 	if (count < 8)
 		return 0;
 
 	spin_unlock_irq(&ff->lock);
-
 	if (copy_to_user(buf, &type, sizeof(type)))
-		return -EFAULT;
-
+		ret = -EFAULT;
 	spin_lock_irq(&ff->lock);
+	if (ret)
+		return ret;
 
 	count -= sizeof(type);
 	consumed += sizeof(type);
 
 	while (count >= sizeof(*parser->msgs) && parser->pull_pos != parser->push_pos) {
 		spin_unlock_irq(&ff->lock);
-
 		if (copy_to_user(buf + consumed, parser->msgs + parser->pull_pos,
 				 sizeof(*parser->msgs)))
-			return -EFAULT;
-
+			ret = -EFAULT;
 		spin_lock_irq(&ff->lock);
+		if (ret)
+			return ret;
+
 		++parser->pull_pos;
 		if (parser->pull_pos >= FF400_QUEUE_SIZE)
 			parser->pull_pos = 0;
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH] ALSA: fireface: fix locking bug in ff400_copy_msg_to_user()
@ 2023-01-17 14:17 ` Dan Carpenter
  0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2023-01-17 14:17 UTC (permalink / raw)
  To: Takashi Sakamoto
  Cc: alsa-devel, Clemens Ladisch, kernel-janitors, Takashi Iwai

The ff400_copy_msg_to_user() function drops the spin lock to call
copy_to_user().  However, if the copy_to_user() fails, then it must
take the lock again before returning.  Failure to take the lock leads
to a double unlock in the caller, hwdep_read().

Fixes: acdebd8b4c0c ("ALSA: fireface: implement message parser for Fireface 400")
Signed-off-by: Dan Carpenter <error27@gmail.com>
---
 sound/firewire/fireface/ff-protocol-former.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/sound/firewire/fireface/ff-protocol-former.c b/sound/firewire/fireface/ff-protocol-former.c
index f58008762fe6..fa41de978756 100644
--- a/sound/firewire/fireface/ff-protocol-former.c
+++ b/sound/firewire/fireface/ff-protocol-former.c
@@ -680,28 +680,30 @@ static long ff400_copy_msg_to_user(struct snd_ff *ff, char __user *buf, long cou
 	struct ff400_msg_parser *parser = ff->msg_parser;
 	u32 type = SNDRV_FIREWIRE_EVENT_FF400_MESSAGE;
 	long consumed = 0;
+	int ret = 0;
 
 	if (count < 8)
 		return 0;
 
 	spin_unlock_irq(&ff->lock);
-
 	if (copy_to_user(buf, &type, sizeof(type)))
-		return -EFAULT;
-
+		ret = -EFAULT;
 	spin_lock_irq(&ff->lock);
+	if (ret)
+		return ret;
 
 	count -= sizeof(type);
 	consumed += sizeof(type);
 
 	while (count >= sizeof(*parser->msgs) && parser->pull_pos != parser->push_pos) {
 		spin_unlock_irq(&ff->lock);
-
 		if (copy_to_user(buf + consumed, parser->msgs + parser->pull_pos,
 				 sizeof(*parser->msgs)))
-			return -EFAULT;
-
+			ret = -EFAULT;
 		spin_lock_irq(&ff->lock);
+		if (ret)
+			return ret;
+
 		++parser->pull_pos;
 		if (parser->pull_pos >= FF400_QUEUE_SIZE)
 			parser->pull_pos = 0;
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] ALSA: fireface: fix locking bug in ff400_copy_msg_to_user()
  2023-01-17 14:17 ` Dan Carpenter
@ 2023-01-18  0:08   ` Takashi Sakamoto
  -1 siblings, 0 replies; 6+ messages in thread
From: Takashi Sakamoto @ 2023-01-18  0:08 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: kernel-janitors, alsa-devel, Clemens Ladisch, Takashi Iwai

Hi,

On Tue, Jan 17, 2023 at 05:17:29PM +0300, Dan Carpenter wrote:
> The ff400_copy_msg_to_user() function drops the spin lock to call
> copy_to_user().  However, if the copy_to_user() fails, then it must
> take the lock again before returning.  Failure to take the lock leads
> to a double unlock in the caller, hwdep_read().
> 
> Fixes: acdebd8b4c0c ("ALSA: fireface: implement message parser for Fireface 400")
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
>  sound/firewire/fireface/ff-protocol-former.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)

Indeed. Thanks for your care.

Acked-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

> diff --git a/sound/firewire/fireface/ff-protocol-former.c b/sound/firewire/fireface/ff-protocol-former.c
> index f58008762fe6..fa41de978756 100644
> --- a/sound/firewire/fireface/ff-protocol-former.c
> +++ b/sound/firewire/fireface/ff-protocol-former.c
> @@ -680,28 +680,30 @@ static long ff400_copy_msg_to_user(struct snd_ff *ff, char __user *buf, long cou
>  	struct ff400_msg_parser *parser = ff->msg_parser;
>  	u32 type = SNDRV_FIREWIRE_EVENT_FF400_MESSAGE;
>  	long consumed = 0;
> +	int ret = 0;
>  
>  	if (count < 8)
>  		return 0;
>  
>  	spin_unlock_irq(&ff->lock);
> -
>  	if (copy_to_user(buf, &type, sizeof(type)))
> -		return -EFAULT;
> -
> +		ret = -EFAULT;
>  	spin_lock_irq(&ff->lock);
> +	if (ret)
> +		return ret;
>  
>  	count -= sizeof(type);
>  	consumed += sizeof(type);
>  
>  	while (count >= sizeof(*parser->msgs) && parser->pull_pos != parser->push_pos) {
>  		spin_unlock_irq(&ff->lock);
> -
>  		if (copy_to_user(buf + consumed, parser->msgs + parser->pull_pos,
>  				 sizeof(*parser->msgs)))
> -			return -EFAULT;
> -
> +			ret = -EFAULT;
>  		spin_lock_irq(&ff->lock);
> +		if (ret)
> +			return ret;
> +
>  		++parser->pull_pos;
>  		if (parser->pull_pos >= FF400_QUEUE_SIZE)
>  			parser->pull_pos = 0;
> -- 
> 2.35.1
 

Regards

Takashi Sakamoto

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] ALSA: fireface: fix locking bug in ff400_copy_msg_to_user()
@ 2023-01-18  0:08   ` Takashi Sakamoto
  0 siblings, 0 replies; 6+ messages in thread
From: Takashi Sakamoto @ 2023-01-18  0:08 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Clemens Ladisch, Jaroslav Kysela, Takashi Iwai, alsa-devel,
	kernel-janitors

Hi,

On Tue, Jan 17, 2023 at 05:17:29PM +0300, Dan Carpenter wrote:
> The ff400_copy_msg_to_user() function drops the spin lock to call
> copy_to_user().  However, if the copy_to_user() fails, then it must
> take the lock again before returning.  Failure to take the lock leads
> to a double unlock in the caller, hwdep_read().
> 
> Fixes: acdebd8b4c0c ("ALSA: fireface: implement message parser for Fireface 400")
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
>  sound/firewire/fireface/ff-protocol-former.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)

Indeed. Thanks for your care.

Acked-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

> diff --git a/sound/firewire/fireface/ff-protocol-former.c b/sound/firewire/fireface/ff-protocol-former.c
> index f58008762fe6..fa41de978756 100644
> --- a/sound/firewire/fireface/ff-protocol-former.c
> +++ b/sound/firewire/fireface/ff-protocol-former.c
> @@ -680,28 +680,30 @@ static long ff400_copy_msg_to_user(struct snd_ff *ff, char __user *buf, long cou
>  	struct ff400_msg_parser *parser = ff->msg_parser;
>  	u32 type = SNDRV_FIREWIRE_EVENT_FF400_MESSAGE;
>  	long consumed = 0;
> +	int ret = 0;
>  
>  	if (count < 8)
>  		return 0;
>  
>  	spin_unlock_irq(&ff->lock);
> -
>  	if (copy_to_user(buf, &type, sizeof(type)))
> -		return -EFAULT;
> -
> +		ret = -EFAULT;
>  	spin_lock_irq(&ff->lock);
> +	if (ret)
> +		return ret;
>  
>  	count -= sizeof(type);
>  	consumed += sizeof(type);
>  
>  	while (count >= sizeof(*parser->msgs) && parser->pull_pos != parser->push_pos) {
>  		spin_unlock_irq(&ff->lock);
> -
>  		if (copy_to_user(buf + consumed, parser->msgs + parser->pull_pos,
>  				 sizeof(*parser->msgs)))
> -			return -EFAULT;
> -
> +			ret = -EFAULT;
>  		spin_lock_irq(&ff->lock);
> +		if (ret)
> +			return ret;
> +
>  		++parser->pull_pos;
>  		if (parser->pull_pos >= FF400_QUEUE_SIZE)
>  			parser->pull_pos = 0;
> -- 
> 2.35.1
 

Regards

Takashi Sakamoto

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] ALSA: fireface: fix locking bug in ff400_copy_msg_to_user()
  2023-01-17 14:17 ` Dan Carpenter
@ 2023-01-18 16:50   ` Takashi Iwai
  -1 siblings, 0 replies; 6+ messages in thread
From: Takashi Iwai @ 2023-01-18 16:50 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Takashi Sakamoto, Clemens Ladisch, Jaroslav Kysela, Takashi Iwai,
	alsa-devel, kernel-janitors

On Tue, 17 Jan 2023 15:17:29 +0100,
Dan Carpenter wrote:
> 
> The ff400_copy_msg_to_user() function drops the spin lock to call
> copy_to_user().  However, if the copy_to_user() fails, then it must
> take the lock again before returning.  Failure to take the lock leads
> to a double unlock in the caller, hwdep_read().
> 
> Fixes: acdebd8b4c0c ("ALSA: fireface: implement message parser for Fireface 400")
> Signed-off-by: Dan Carpenter <error27@gmail.com>

Thanks, applied.


Takashi

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] ALSA: fireface: fix locking bug in ff400_copy_msg_to_user()
@ 2023-01-18 16:50   ` Takashi Iwai
  0 siblings, 0 replies; 6+ messages in thread
From: Takashi Iwai @ 2023-01-18 16:50 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: alsa-devel, Clemens Ladisch, kernel-janitors, Takashi Iwai

On Tue, 17 Jan 2023 15:17:29 +0100,
Dan Carpenter wrote:
> 
> The ff400_copy_msg_to_user() function drops the spin lock to call
> copy_to_user().  However, if the copy_to_user() fails, then it must
> take the lock again before returning.  Failure to take the lock leads
> to a double unlock in the caller, hwdep_read().
> 
> Fixes: acdebd8b4c0c ("ALSA: fireface: implement message parser for Fireface 400")
> Signed-off-by: Dan Carpenter <error27@gmail.com>

Thanks, applied.


Takashi

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-01-18 16:51 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-17 14:17 [PATCH] ALSA: fireface: fix locking bug in ff400_copy_msg_to_user() Dan Carpenter
2023-01-17 14:17 ` Dan Carpenter
2023-01-18  0:08 ` Takashi Sakamoto
2023-01-18  0:08   ` Takashi Sakamoto
2023-01-18 16:50 ` Takashi Iwai
2023-01-18 16:50   ` Takashi Iwai

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.