diff mbox series

ALSA: firewire-motu: fix invalid memory access when operating hwdep character device

Message ID 20211020042555.40866-1-o-takashi@sakamocchi.jp (mailing list archive)
State New, archived
Headers show
Series ALSA: firewire-motu: fix invalid memory access when operating hwdep character device | expand

Commit Message

Takashi Sakamoto Oct. 20, 2021, 4:25 a.m. UTC 
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>
---
 sound/firewire/motu/motu-hwdep.c | 72 ++++++++++++++++++++------------
 1 file changed, 45 insertions(+), 27 deletions(-)

Comments

Takashi Iwai Oct. 20, 2021, 5:40 a.m. UTC | #1 
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
>
Takashi Sakamoto Oct. 21, 2021, 2:11 p.m. UTC | #2 
Hi,

On Wed, Oct 20, 2021 at 07:40:46AM +0200, Takashi Iwai wrote:
> 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?

Indeed. It's simpler than my patch.

When posted, I considered about pre-condition in context of design by
contract. I programmed motu-register-dsp-message-parser.c so that
callers of extern functions should guarantee the target is register
DSP models.

> > +// 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)))

The second argument should have been 'event'...

Anyway I'll use a bit more time to consider about what is better to fix the
bug.


Thanks

Takashi Sakamoto
kernel test robot Oct. 25, 2021, 3:55 p.m. UTC | #3 
Hi Takashi,

I love your patch! Yet something to improve:

[auto build test ERROR on tiwai-sound/for-next]
[also build test ERROR on next-20211025]
[cannot apply to v5.15-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Takashi-Sakamoto/ALSA-firewire-motu-fix-invalid-memory-access-when-operating-hwdep-character-device/20211020-122834
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
config: arc-randconfig-r043-20211020 (attached as .config)
compiler: arc-elf-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/0105a381c2c6677118fca8f30b96778d590e2173
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Takashi-Sakamoto/ALSA-firewire-motu-fix-invalid-memory-access-when-operating-hwdep-character-device/20211020-122834
        git checkout 0105a381c2c6677118fca8f30b96778d590e2173
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arc SHELL=/bin/bash sound/firewire/motu/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/asm-generic/current.h:5,
                    from arch/arc/include/asm/current.h:20,
                    from include/linux/sched.h:12,
                    from include/linux/ratelimit.h:6,
                    from include/linux/dev_printk.h:16,
                    from include/linux/device.h:15,
                    from sound/firewire/motu/motu.h:11,
                    from sound/firewire/motu/motu-hwdep.c:17:
   In function 'check_copy_size',
       inlined from 'copy_to_user' at include/linux/uaccess.h:199:6,
       inlined from 'copy_dsp_event_to_user' at sound/firewire/motu/motu-hwdep.c:49:7,
       inlined from 'hwdep_read' at sound/firewire/motu/motu-hwdep.c:101:11:
>> include/linux/thread_info.h:211:25: error: call to '__bad_copy_from' declared with attribute error: copy source size is too small
     211 |                         __bad_copy_from();
         |                         ^~~~~~~~~~~~~~~~~


vim +/__bad_copy_from +211 include/linux/thread_info.h

b0377fedb65280 Al Viro   2017-06-29  202  
9dd819a15162f8 Kees Cook 2019-09-25  203  static __always_inline __must_check bool
b0377fedb65280 Al Viro   2017-06-29  204  check_copy_size(const void *addr, size_t bytes, bool is_source)
b0377fedb65280 Al Viro   2017-06-29  205  {
b0377fedb65280 Al Viro   2017-06-29  206  	int sz = __compiletime_object_size(addr);
b0377fedb65280 Al Viro   2017-06-29  207  	if (unlikely(sz >= 0 && sz < bytes)) {
b0377fedb65280 Al Viro   2017-06-29  208  		if (!__builtin_constant_p(bytes))
b0377fedb65280 Al Viro   2017-06-29  209  			copy_overflow(sz, bytes);
b0377fedb65280 Al Viro   2017-06-29  210  		else if (is_source)
b0377fedb65280 Al Viro   2017-06-29 @211  			__bad_copy_from();
b0377fedb65280 Al Viro   2017-06-29  212  		else
b0377fedb65280 Al Viro   2017-06-29  213  			__bad_copy_to();
b0377fedb65280 Al Viro   2017-06-29  214  		return false;
b0377fedb65280 Al Viro   2017-06-29  215  	}
6d13de1489b6bf Kees Cook 2019-12-04  216  	if (WARN_ON_ONCE(bytes > INT_MAX))
6d13de1489b6bf Kees Cook 2019-12-04  217  		return false;
b0377fedb65280 Al Viro   2017-06-29  218  	check_object_size(addr, bytes, is_source);
b0377fedb65280 Al Viro   2017-06-29  219  	return true;
b0377fedb65280 Al Viro   2017-06-29  220  }
b0377fedb65280 Al Viro   2017-06-29  221  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

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;