Re: [PATCH] ALSA: firewire-motu: fix invalid memory access when operating hwdep character device

[Takashi Iwai <tiwai@suse.de>]

On Wed, 20 Oct 2021 06:25:55 +0200,
Takashi Sakamoto wrote:
> 
> ALSA firewire-motu driver recently got support for event notification via
> ALSA HwDep interface for register DSP models. However, when polling ALSA
> HwDep cdev, the driver can cause null pointer dereference for the other
> models due to accessing to unallocated memory or uninitialized memory.
> 
> This commit fixes the bug by check the type of model before accessing to
> the memory.
> 
> Fixes: 634ec0b2906e ("ALSA: firewire-motu: notify event for parameter change in register DSP model")
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

Wouldn't it be simpler to add the flag check in
snd_motu_register_dsp_message_parser_count_event() and return 0 if
SND_MOTU_SPEC_REGISTER_DSP isn't set there?


thanks,

Takashi

> ---
>  sound/firewire/motu/motu-hwdep.c | 72 ++++++++++++++++++++------------
>  1 file changed, 45 insertions(+), 27 deletions(-)
> 
> diff --git a/sound/firewire/motu/motu-hwdep.c b/sound/firewire/motu/motu-hwdep.c
> index 9c2e457ce692..ae2d01ddc8d3 100644
> --- a/sound/firewire/motu/motu-hwdep.c
> +++ b/sound/firewire/motu/motu-hwdep.c
> @@ -16,6 +16,47 @@
>  
>  #include "motu.h"
>  
> +static bool has_dsp_event(struct snd_motu *motu)
> +{
> +	if (motu->spec->flags & SND_MOTU_SPEC_REGISTER_DSP)
> +		return (snd_motu_register_dsp_message_parser_count_event(motu) > 0);
> +	else
> +		return false;
> +}
> +
> +// NOTE: Take care of page fault due to accessing to userspace.
> +static long copy_dsp_event_to_user(struct snd_motu *motu, char __user *buf, long count,
> +				   struct snd_firewire_event_motu_register_dsp_change *event)
> +{
> +	if (motu->spec->flags & SND_MOTU_SPEC_REGISTER_DSP) {
> +		size_t consumed = 0;
> +		u32 __user *ptr;
> +		u32 ev;
> +
> +		// Header is filled later.
> +		consumed += sizeof(*event);
> +
> +		while (consumed < count &&
> +		       snd_motu_register_dsp_message_parser_copy_event(motu, &ev)) {
> +			ptr = (u32 __user *)(buf + consumed);
> +			if (put_user(ev, ptr))
> +				return -EFAULT;
> +			consumed += sizeof(ev);
> +		}
> +
> +		event->type = SNDRV_FIREWIRE_EVENT_MOTU_REGISTER_DSP_CHANGE;
> +		event->count = (consumed - sizeof(*event)) / 4;
> +		if (copy_to_user(buf, &event, sizeof(*event)))
> +			return -EFAULT;
> +
> +		count = consumed;
> +	} else {
> +		count = 0;
> +	}
> +
> +	return count;
> +}
> +
>  static long hwdep_read(struct snd_hwdep *hwdep, char __user *buf, long count,
>  		       loff_t *offset)
>  {
> @@ -25,8 +66,7 @@ static long hwdep_read(struct snd_hwdep *hwdep, char __user *buf, long count,
>  
>  	spin_lock_irq(&motu->lock);
>  
> -	while (!motu->dev_lock_changed && motu->msg == 0 &&
> -			snd_motu_register_dsp_message_parser_count_event(motu) == 0) {
> +	while (!motu->dev_lock_changed && motu->msg == 0 && !has_dsp_event(motu)) {
>  		prepare_to_wait(&motu->hwdep_wait, &wait, TASK_INTERRUPTIBLE);
>  		spin_unlock_irq(&motu->lock);
>  		schedule();
> @@ -55,31 +95,10 @@ static long hwdep_read(struct snd_hwdep *hwdep, char __user *buf, long count,
>  		count = min_t(long, count, sizeof(event));
>  		if (copy_to_user(buf, &event, count))
>  			return -EFAULT;
> -	} else if (snd_motu_register_dsp_message_parser_count_event(motu) > 0) {
> -		size_t consumed = 0;
> -		u32 __user *ptr;
> -		u32 ev;
> -
> +	} else if (has_dsp_event(motu)) {
>  		spin_unlock_irq(&motu->lock);
>  
> -		// Header is filled later.
> -		consumed += sizeof(event.motu_register_dsp_change);
> -
> -		while (consumed < count &&
> -		       snd_motu_register_dsp_message_parser_copy_event(motu, &ev)) {
> -			ptr = (u32 __user *)(buf + consumed);
> -			if (put_user(ev, ptr))
> -				return -EFAULT;
> -			consumed += sizeof(ev);
> -		}
> -
> -		event.motu_register_dsp_change.type = SNDRV_FIREWIRE_EVENT_MOTU_REGISTER_DSP_CHANGE;
> -		event.motu_register_dsp_change.count =
> -			(consumed - sizeof(event.motu_register_dsp_change)) / 4;
> -		if (copy_to_user(buf, &event, sizeof(event.motu_register_dsp_change)))
> -			return -EFAULT;
> -
> -		count = consumed;
> +		count = copy_dsp_event_to_user(motu, buf, count, &event.motu_register_dsp_change);
>  	}
>  
>  	return count;
> @@ -94,8 +113,7 @@ static __poll_t hwdep_poll(struct snd_hwdep *hwdep, struct file *file,
>  	poll_wait(file, &motu->hwdep_wait, wait);
>  
>  	spin_lock_irq(&motu->lock);
> -	if (motu->dev_lock_changed || motu->msg ||
> -	    snd_motu_register_dsp_message_parser_count_event(motu) > 0)
> +	if (motu->dev_lock_changed || motu->msg || has_dsp_event(motu))
>  		events = EPOLLIN | EPOLLRDNORM;
>  	else
>  		events = 0;
> -- 
> 2.30.2
>