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

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

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...)


Thanks

Takashi Sakamoto