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@xxxxxxxxxxxxx>
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
>
