ALSA: usbusx2y: fix in04_last array size causing slab OOB read

From: Tristan Madani <[email protected]>

The in04_last array in struct usx2ydev is declared as char[24], but in04_buf (the source for memcpy) is allocated with kmalloc(21). In i_usx2y_in04_int(), when ctl_snapshot_last == -2 (initialization path):

    memcpy(usx2y->in04_last, usx2y->in04_buf, sizeof(usx2y->in04_last));

This copies 24 bytes from a 21-byte slab allocation, reading 3 bytes past the end of the kmalloc-32 object.

The comparison loop already uses the correct bound of 21:

    for (i = 0; i < 21; i++) {

Fix by reducing the in04_last array to 21 bytes, matching the actual USB interrupt transfer size and the in04_buf allocation.

---
 sound/usb/usx2y/usbusx2y.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 6a76d04bf1c7d..266a3483ab81c 100644
@@ -55,7 +55,7 @@ struct usx2ydev {
 	int			stride;
 	struct urb		*in04_urb;
 	void			*in04_buf;
-	char			in04_last[24];
+	char			in04_last[21];
 	unsigned int		in04_int_calls;
 	struct snd_usx2y_urb_seq	*us04;
 	wait_queue_head_t	in04_wait_queue;

2.47.3

From: Tristan Madani <[email protected]>

i_usx2y_in04_int() processes the interrupt URB data without checking urb->actual_length. A malfunctioning USB device could send a short transfer, causing the handler to process uninitialized heap data from the kmalloc-allocated in04_buf.

This is problematic because in04_buf is allocated with kmalloc() (not kzalloc()), so uninitialized slab data may be present before the first full transfer. The data is then copied to us428ctls->ctl_snapshot[], which is mmap-accessible to userspace via snd_us428ctls_mmap().

Fix by:
1. Using kzalloc() for in04_buf to zero-initialize the buffer
2. Adding an actual_length check at the start of the callback to skip processing on short transfers while still resubmitting the URB

---
 sound/usb/usx2y/usbusx2y.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

index 4190227c5a2a5..5744a873aac26 100644
@@ -189,6 +189,9 @@ static void i_usx2y_in04_int(struct urb *urb)
 		return;
 	}
 
+	if (urb->actual_length < 21)
+		goto resubmit;
+
 	if (us428ctls) {
 		diff = -1;
 		if (us428ctls->ctl_snapshot_last == -2) {

@@ -253,6 +256,7 @@ static void i_usx2y_in04_int(struct urb *urb)
 	if (err)
 		dev_err(&urb->dev->dev, "in04_int() usb_submit_urb err=%i\n", err);
 
+resubmit:
 	urb->dev = usx2y->dev;
 	usb_submit_urb(urb, GFP_ATOMIC);
 }

@@ -305,7 +309,7 @@ int usx2y_in04_init(struct usx2ydev *usx2y)
 		goto error;
 	}
 
-	usx2y->in04_buf = kmalloc(21, GFP_KERNEL);
+	usx2y->in04_buf = kzalloc(21, GFP_KERNEL);
 	if (!usx2y->in04_buf) {
 		err = -ENOMEM;
 		goto error;

2.47.3

Better to introduce a constant definition and use it in all places instead of magic numbers. Could you rework your patches with it?

thanks,

Takashi