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

[Takashi Sakamoto <o-takashi@sakamocchi.jp>]

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