[v2] ALSA: usb-mixer: us16x08: validate meter packet indices

[PATCH v2] ALSA: usb-mixer: us16x08: validate meter packet indices

Posted by Shipei Qu 1 month, 3 weeks ago

Hi,

resending with a properly formatted diff (the previous email had a malformed
patch header). The change itself is the same: while fuzzing a USB gadget that
emulates a Tascam US-16x08 we found that get_meter_levels_from_urb() in
mixer_us16x08.c uses a channel index taken directly from the 64-byte meter
packet to index meter_level[], comp_level[] and master_level[] without any
bounds checking. A malformed packet can therefore cause out-of-bounds writes in
the snd_us16x08_meter_store.

A malicious USB audio device (or USB gadget implementation) that pretends to be
a US-16x08-compatible interface can trigger this by sending crafted meter
packets. We have a small USB gadget-based PoC for this behaviour and can share
it if that would be helpful.

This driver is used by common distributions (e.g. Ubuntu) when a US-16x08 or
compatible USB audio device is present. The same pattern is present in current
mainline kernels.

This issue was first reported via security@kernel.org. The kernel security team
explained that, in the upstream threat model, USB endpoints are expected to be
trusted (i.e. only trusted devices should be bound to drivers), so they
consider this a normal bug rather than a security vulnerability, and asked us
to send a fix to the development lists. The patch below adds simple range
checks before updating these arrays.

Reported-by: DARKNAVY (@DarkNavyOrg) <vr@darknavy.com>
Signed-off-by: Shipei Qu <qu@darknavy.com>
---
 sound/usb/mixer_us16x08.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/sound/usb/mixer_us16x08.c b/sound/usb/mixer_us16x08.c
index 1c5712c31..f9df40730 100644
--- a/sound/usb/mixer_us16x08.c
+++ b/sound/usb/mixer_us16x08.c
@@ -655,17 +655,25 @@ static void get_meter_levels_from_urb(int s,
 	u8 *meter_urb)
 {
 	int val = MUC2(meter_urb, s) + (MUC3(meter_urb, s) << 8);
+	int ch = MUB2(meter_urb, s) - 1;
+
+	if (ch < 0)
+		return;
 
 	if (MUA0(meter_urb, s) == 0x61 && MUA1(meter_urb, s) == 0x02 &&
 		MUA2(meter_urb, s) == 0x04 && MUB0(meter_urb, s) == 0x62) {
-		if (MUC0(meter_urb, s) == 0x72)
-			store->meter_level[MUB2(meter_urb, s) - 1] = val;
-		if (MUC0(meter_urb, s) == 0xb2)
-			store->comp_level[MUB2(meter_urb, s) - 1] = val;
+		if (ch < SND_US16X08_MAX_CHANNELS) {
+			if (MUC0(meter_urb, s) == 0x72)
+				store->meter_level[ch] = val;
+			if (MUC0(meter_urb, s) == 0xb2)
+				store->comp_level[ch] = val;
+		}
 	}
 	if (MUA0(meter_urb, s) == 0x61 && MUA1(meter_urb, s) == 0x02 &&
-		MUA2(meter_urb, s) == 0x02 && MUB0(meter_urb, s) == 0x62)
-		store->master_level[MUB2(meter_urb, s) - 1] = val;
+		MUA2(meter_urb, s) == 0x02 && MUB0(meter_urb, s) == 0x62) {
+		if (ch < ARRAY_SIZE(store->master_level))
+			store->master_level[ch] = val;
+	}
 }
 
 /* Function to retrieve current meter values from the device.
-- 
2.45.1

Re: [PATCH v2] ALSA: usb-mixer: us16x08: validate meter packet indices

Posted by Takashi Iwai 1 month, 3 weeks ago

On Tue, 16 Dec 2025 07:01:56 +0100,
Shipei Qu wrote:
> 
> Hi,
> 
> resending with a properly formatted diff (the previous email had a malformed
> patch header). The change itself is the same: while fuzzing a USB gadget that
> emulates a Tascam US-16x08 we found that get_meter_levels_from_urb() in
> mixer_us16x08.c uses a channel index taken directly from the 64-byte meter
> packet to index meter_level[], comp_level[] and master_level[] without any
> bounds checking. A malformed packet can therefore cause out-of-bounds writes in
> the snd_us16x08_meter_store.
> 
> A malicious USB audio device (or USB gadget implementation) that pretends to be
> a US-16x08-compatible interface can trigger this by sending crafted meter
> packets. We have a small USB gadget-based PoC for this behaviour and can share
> it if that would be helpful.
> 
> This driver is used by common distributions (e.g. Ubuntu) when a US-16x08 or
> compatible USB audio device is present. The same pattern is present in current
> mainline kernels.
> 
> This issue was first reported via security@kernel.org. The kernel security team
> explained that, in the upstream threat model, USB endpoints are expected to be
> trusted (i.e. only trusted devices should be bound to drivers), so they
> consider this a normal bug rather than a security vulnerability, and asked us
> to send a fix to the development lists. The patch below adds simple range
> checks before updating these arrays.
> 
> Reported-by: DARKNAVY (@DarkNavyOrg) <vr@darknavy.com>
> Signed-off-by: Shipei Qu <qu@darknavy.com>

Thanks, now it looks applicable, and the code change itself looks
good.

But, could you try to rephrase the patch description above?  It'll be
used directly as the commit message.  So it should be well descriptive
about the bug and the fix, while it should concentrate on the
technical issues.

You can check other commits how they look like, and also see
Documentation/process/submitting-patches.rst.


thanks,

Takashi

> ---
>  sound/usb/mixer_us16x08.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/sound/usb/mixer_us16x08.c b/sound/usb/mixer_us16x08.c
> index 1c5712c31..f9df40730 100644
> --- a/sound/usb/mixer_us16x08.c
> +++ b/sound/usb/mixer_us16x08.c
> @@ -655,17 +655,25 @@ static void get_meter_levels_from_urb(int s,
>  	u8 *meter_urb)
>  {
>  	int val = MUC2(meter_urb, s) + (MUC3(meter_urb, s) << 8);
> +	int ch = MUB2(meter_urb, s) - 1;
> +
> +	if (ch < 0)
> +		return;
>  
>  	if (MUA0(meter_urb, s) == 0x61 && MUA1(meter_urb, s) == 0x02 &&
>  		MUA2(meter_urb, s) == 0x04 && MUB0(meter_urb, s) == 0x62) {
> -		if (MUC0(meter_urb, s) == 0x72)
> -			store->meter_level[MUB2(meter_urb, s) - 1] = val;
> -		if (MUC0(meter_urb, s) == 0xb2)
> -			store->comp_level[MUB2(meter_urb, s) - 1] = val;
> +		if (ch < SND_US16X08_MAX_CHANNELS) {
> +			if (MUC0(meter_urb, s) == 0x72)
> +				store->meter_level[ch] = val;
> +			if (MUC0(meter_urb, s) == 0xb2)
> +				store->comp_level[ch] = val;
> +		}
>  	}
>  	if (MUA0(meter_urb, s) == 0x61 && MUA1(meter_urb, s) == 0x02 &&
> -		MUA2(meter_urb, s) == 0x02 && MUB0(meter_urb, s) == 0x62)
> -		store->master_level[MUB2(meter_urb, s) - 1] = val;
> +		MUA2(meter_urb, s) == 0x02 && MUB0(meter_urb, s) == 0x62) {
> +		if (ch < ARRAY_SIZE(store->master_level))
> +			store->master_level[ch] = val;
> +	}
>  }
>  
>  /* Function to retrieve current meter values from the device.
> -- 
> 2.45.1

[PATCH v3] ALSA: usb-mixer: us16x08: validate meter packet indices

Posted by Shipei Qu 1 month, 3 weeks ago

get_meter_levels_from_urb() parses the 64-byte meter packets sent by
the device and fills the per-channel arrays meter_level[],
comp_level[] and master_level[] in struct snd_us16x08_meter_store.

Currently the function derives the channel index directly from the
meter packet (MUB2(meter_urb, s) - 1) and uses it to index those
arrays without validating the range. If the packet contains a
negative or out-of-range channel number, the driver may write past
the end of these arrays.

Introduce a local channel variable and validate it before updating the
arrays. We reject negative indices, limit meter_level[] and
comp_level[] to SND_US16X08_MAX_CHANNELS, and guard master_level[]
updates with ARRAY_SIZE(master_level).

Reported-by: DARKNAVY (@DarkNavyOrg) <vr@darknavy.com>
Signed-off-by: Shipei Qu <qu@darknavy.com>
---
v3: rephrase commit description
v2: resend with properly formatted diff

 sound/usb/mixer_us16x08.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/sound/usb/mixer_us16x08.c b/sound/usb/mixer_us16x08.c
index 1c5712c31..f9df40730 100644
--- a/sound/usb/mixer_us16x08.c
+++ b/sound/usb/mixer_us16x08.c
@@ -655,17 +655,25 @@ static void get_meter_levels_from_urb(int s,
 	u8 *meter_urb)
 {
 	int val = MUC2(meter_urb, s) + (MUC3(meter_urb, s) << 8);
+	int ch = MUB2(meter_urb, s) - 1;
+
+	if (ch < 0)
+		return;
 
 	if (MUA0(meter_urb, s) == 0x61 && MUA1(meter_urb, s) == 0x02 &&
 		MUA2(meter_urb, s) == 0x04 && MUB0(meter_urb, s) == 0x62) {
-		if (MUC0(meter_urb, s) == 0x72)
-			store->meter_level[MUB2(meter_urb, s) - 1] = val;
-		if (MUC0(meter_urb, s) == 0xb2)
-			store->comp_level[MUB2(meter_urb, s) - 1] = val;
+		if (ch < SND_US16X08_MAX_CHANNELS) {
+			if (MUC0(meter_urb, s) == 0x72)
+				store->meter_level[ch] = val;
+			if (MUC0(meter_urb, s) == 0xb2)
+				store->comp_level[ch] = val;
+		}
 	}
 	if (MUA0(meter_urb, s) == 0x61 && MUA1(meter_urb, s) == 0x02 &&
-		MUA2(meter_urb, s) == 0x02 && MUB0(meter_urb, s) == 0x62)
-		store->master_level[MUB2(meter_urb, s) - 1] = val;
+		MUA2(meter_urb, s) == 0x02 && MUB0(meter_urb, s) == 0x62) {
+		if (ch < ARRAY_SIZE(store->master_level))
+			store->master_level[ch] = val;
+	}
 }
 
 /* Function to retrieve current meter values from the device.
-- 
2.45.1

Re: [PATCH v3] ALSA: usb-mixer: us16x08: validate meter packet indices

Posted by Takashi Iwai 1 month, 3 weeks ago

On Wed, 17 Dec 2025 03:46:30 +0100,
Shipei Qu wrote:
> 
> get_meter_levels_from_urb() parses the 64-byte meter packets sent by
> the device and fills the per-channel arrays meter_level[],
> comp_level[] and master_level[] in struct snd_us16x08_meter_store.
> 
> Currently the function derives the channel index directly from the
> meter packet (MUB2(meter_urb, s) - 1) and uses it to index those
> arrays without validating the range. If the packet contains a
> negative or out-of-range channel number, the driver may write past
> the end of these arrays.
> 
> Introduce a local channel variable and validate it before updating the
> arrays. We reject negative indices, limit meter_level[] and
> comp_level[] to SND_US16X08_MAX_CHANNELS, and guard master_level[]
> updates with ARRAY_SIZE(master_level).
> 
> Reported-by: DARKNAVY (@DarkNavyOrg) <vr@darknavy.com>
> Signed-off-by: Shipei Qu <qu@darknavy.com>

Applied now.  Thanks.


Takashi