Re: [PATCH v2] ALSA: firewire-motu: fix buffer overflow in hwdep read for DSP events

[Takashi Iwai <tiwai@suse.de>]

On Sun, 14 Dec 2025 02:57:41 +0100,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> Sorry for my late reply, but I always postpone my review work during the
> merge window.
> 
> On Mon, Dec 08, 2025 at 08:09:09PM +0800, Junrui Luo wrote:
> > The DSP event handling code in hwdep_read() could write more bytes to
> > the user buffer than requested, when a user provides a buffer smaller
> > than the event header size (8 bytes).
> > 
> > In the put_user() loop that copies event data, when the user buffer
> > size is not aligned to 4 bytes, it could overwrite beyond the buffer
> > boundary.
> > 
> > Fix by:
> > - using min_t() to clamp the copy size
> > - Adding a bounds check before put_user()
> > 
> > This ensures we never write more than the user requested.
> > 
> > Fixes: 634ec0b2906e ("ALSA: firewire-motu: notify event for parameter change in register DSP model")
> > Signed-off-by: Junrui Luo <moonafterrain@outlook.com>
> > ---
> > Changes in v2:
> > - Also fix the similar issue in the put_user() loop suggested by Takashi Iwai
> > - Link to v1: https://lore.kernel.org/all/SYBPR01MB78810656377E79E58350D951AFD9A@SYBPR01MB7881.ausprd01.prod.outlook.com/T/#u
> > ---
> >  sound/firewire/motu/motu-hwdep.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> Indeed, I admit my below lines are enough rough. Thanks for your correction.
> 
> > diff --git a/sound/firewire/motu/motu-hwdep.c b/sound/firewire/motu/motu-hwdep.c
> > index 981c19430cb0..89dc436a0652 100644
> > --- a/sound/firewire/motu/motu-hwdep.c
> > +++ b/sound/firewire/motu/motu-hwdep.c
> > @@ -75,7 +75,7 @@ static long hwdep_read(struct snd_hwdep *hwdep, char __user *buf, long count,
> >  		while (consumed < count &&
> >  		       snd_motu_register_dsp_message_parser_copy_event(motu, &ev)) {
> >  			ptr = (u32 __user *)(buf + consumed);
> > -			if (put_user(ev, ptr))
> > +			if (consumed + sizeof(ev) > count || put_user(ev, ptr))
> >  				return -EFAULT;
> 
> I think it is too strong when there is no rest to store the event data
> after filling some events on the given buffer. Let us move the size
> check to the loop condition? Like:
> 
> ```
> while (consumed + sizeof(ev) < count &&
>    snd_motu_register_dsp_message_parser_copy_event(motu, &ev)) {
>     u32 __user *ptr = (u32 __user *)(buf + consumed);
> 
>     if (put_user(ev, ptr))
>         return -EFAULT;
> 
>     consumed += sizeof(ev);
> }
> ```
> 
> We can guarantee the space for one event at least in every iteration of
> loop.
> 
> >  			consumed += sizeof(ev);
> >  		}
> > @@ -83,10 +83,11 @@ static long hwdep_read(struct snd_hwdep *hwdep, char __user *buf, long count,
> >  		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)))
> > +		if (copy_to_user(buf, &event,
> > +				 min_t(long, count, sizeof(event.motu_register_dsp_change))))
> >  			return -EFAULT;
> >  
> > -		count = consumed;
> > +		count = min_t(long, count, consumed);
> >  	} else {
> >  		spin_unlock_irq(&motu->lock);
> 
> In my opinion, if there is no space for data header, the overall copying
> should be canceled in advance, thus:
> 
> ```
> } else if (has_dsp_event(motu)) {
>     size_t consumed = 0;
>     u32 ev;
> 
>     spin_unlock_irq(&motu->lock);
> 
>     if (count < sizeof(event.motu_register_dsp_change))
>             return 0;
> 
>     // Header is filled later.
>     consumed += sizeof(event.motu_register_dsp_change);
>     ...
> ```
> 
> (In the case of failure to access to userspace for the data header, the
> underlying layer for register DSP event should have peek/ack mechanism,
> however I'm too lazy...)

It's a question of API design, after all.  If the size shortage
shouldn't be  treated as an error, it can be as your suggestion.
Feel free to submit the change :)


thanks,

Takashi