Re: [PATCH v2 2/2 RESEND] usb: gadget: f_uac2: allow explicit channel count independent of channel mask

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] usb: gadget: u_audio: heap buffer overflow in u_audio_iso_complete() due to missing bounds check
- [High] usb: gadget: f_uac2: u16 truncation in get_max_bw_for_bint() leads to undersized wMaxPacketSize
- [High] usb: gadget: f_uac2: memory leak and race from shared static globals
--

commit c9264c2b1a112c6d9a2f079577364a2e863c803f
Author: Kyle Farrell <kaf@nwlink.com>

usb: gadget: f_uac2: allow explicit channel count independent of channel mask

The UAC2 gadget driver currently derives bNrChannels from the number
of set bits in bmChannelConfig. Since the bmChannelConfig
channel-location bitmap provides only 27 predefined channel locations,
this limits playback and capture streams to 27 channels. UAC2 permits
bNrChannels values up to 255, independent of bmChannelConfig.

> diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c
> index 46255ad0a9cfa..c7801bb8765e6 100644
> --- a/drivers/usb/gadget/function/f_uac2.c
> +++ b/drivers/usb/gadget/function/f_uac2.c
[ ... ]
> @@ -689,11 +699,11 @@ static int get_max_bw_for_bint(const struct f_uac2_opts *uac2_opts,
>  		// Win10 requires max packet size + 1 frame
>  		srate = srate * (1000 + uac2_opts->fb_max) / 1000;
>  		// updated srate is always bigger, therefore DIV_ROUND_UP always yields +1
> -		max_size_bw = num_channels(chmask) * ssize *
> +		max_size_bw = channels * ssize *
>  			(DIV_ROUND_UP(srate, factor / (1 << (bint - 1))));
>  	} else {
>  		// adding 1 frame provision for Win10
> -		max_size_bw = num_channels(chmask) * ssize *
> +		max_size_bw = channels * ssize *
>  			(DIV_ROUND_UP(srate, factor / (1 << (bint - 1))) + 1);
>  	}
>  	return max_size_bw;

[Severity: High]
Can this overflow the 16-bit capacity of max_size_bw?

With channel counts up to 255 independent of the channel mask, this calculation
can exceed 65535, causing the required bandwidth to be silently truncated.

If this truncates, could it bypass the validation check in
set_ep_max_packet_size_bint() and allow invalid, undersized wMaxPacketSize
dimensions to be programmed:

drivers/usb/gadget/function/f_uac2.c:set_ep_max_packet_size_bint() {
    ...
    if (max_size_bw <= max_size_ep)
        dev_dbg(dev,
            "%s %s: Would use wMaxPacketSize %d and bInterval %d\n",
            speed_names[speed], dir, max_size_bw, bint);
    else {
    ...
}

This would severely undersize downstream ALSA ring buffer chunk allocations
and cause data drops.

[ ... ]
> @@ -1059,12 +1084,12 @@ afunc_bind(struct usb_configuration *cfg, struct usb_function *fn)
>  		return PTR_ERR(us);
>  
>  	if (FUOUT_EN(uac2_opts)) {
> -		out_feature_unit_desc = build_fu_desc(uac2_opts->c_chmask);
> +		out_feature_unit_desc = build_fu_desc(false, uac2_opts);
>  		if (!out_feature_unit_desc)
>  			return -ENOMEM;
>  	}

[Severity: High]
Does dynamically allocating memory and assigning it to the shared static global
out_feature_unit_desc overwrite prior instance allocations if multiple UAC2
instances are bound to a composite gadget via configfs, leaking memory?

[ ... ]
> @@ -1099,13 +1124,13 @@ afunc_bind(struct usb_configuration *cfg, struct usb_function *fn)
>  
>  
>  	/* Initialize the configurable parameters */
> -	usb_out_it_desc.bNrChannels = num_channels(uac2_opts->c_chmask);
> +	usb_out_it_desc.bNrChannels = uac2_num_channels(false, uac2_opts);
>  	usb_out_it_desc.bmChannelConfig = cpu_to_le32(uac2_opts->c_chmask);
> -	io_in_it_desc.bNrChannels = num_channels(uac2_opts->p_chmask);
> +	io_in_it_desc.bNrChannels = uac2_num_channels(true, uac2_opts);

[Severity: High]
Does modifying static descriptor templates like usb_out_it_desc directly
during bind cause data races and corrupted descriptor fields if concurrent
bindings occur?

[Severity: Critical]
With the channel count expanded to 255, could this cause a heap buffer overflow
downstream in drivers/usb/gadget/function/u_audio.c?

When uac->p_framesize is dramatically increased by the larger channel counts,
and residue accumulation triggers in u_audio_iso_complete():

drivers/usb/gadget/function/u_audio.c:u_audio_iso_complete() {
    ...
    if ((unsigned int) div_result >= uac->p_framesize) {
        req->length += uac->p_framesize;
        uac->p_residue_mil -= uac->p_framesize * p_interval_mil;
    ...
    if (unlikely(pending < req->actual)) {
        memcpy(req->buf, runtime->dma_area + hw_ptr, pending);
        memcpy(req->buf + pending, runtime->dma_area, req->actual - pending);
    ...
}

Will this memcpy overflow req->buf by up to p_framesize because req->length
was increased without verifying it stays within ep->maxpacket?

Request buffers are tightly packed at intervals of ep->maxpacket:

drivers/usb/gadget/function/u_audio.c:u_audio_start_playback() {
    ...
    req->buf = prm->rbuf + i * ep->maxpacket;
    ...
}

This means any overflow immediately corrupts the next request's active DMA
memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016162534.384844-1-kaf@nwlink.com?part=2