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

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

[Junrui Luo]

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

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

---
base-commit: 559e608c46553c107dbba19dae0854af7b219400
change-id: 20251208-fixes-2d11c1218a5f

Best regards,
-- 
Junrui Luo <moonafterrain@outlook.com>

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

[Takashi Iwai]

On Mon, 08 Dec 2025 13:09:09 +0100,
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

v1 was already merged.  Please submit an incremental fix on top of
sound.git tree for-linus branch.


thanks,

Takashi

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

[Takashi Sakamoto]

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

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

[Takashi Iwai]

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