Re: [PATCH] usb: gadget: f_uac1_legacy: validate control request size

[Greg KH <gregkh@linuxfoundation.org>]

On Wed, Apr 01, 2026 at 07:46:11PM +0900, Taegu Ha wrote:
> f_audio_complete() copies req->length bytes into a 4-byte stack
> variable:
> 
>   u32 data = 0;
>   memcpy(&data, req->buf, req->length);
> 
> req->length is derived from the host-controlled USB request path,
> which can lead to a stack out-of-bounds write.
> 
> Validate req->actual against the expected payload size for the
> supported control selectors and decode only the expected amount
> of data.
> 
> This avoids copying a host-influenced length into a fixed-size
> stack object.
> 
> Signed-off-by: Taegu Ha <hataegu0826@gmail.com>
> ---
>  drivers/usb/gadget/function/f_uac1_legacy.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_uac1_legacy.c b/drivers/usb/gadget/function/f_uac1_legacy.c
> index a0c953a99727..6d6fe5db99f5 100644
> --- a/drivers/usb/gadget/function/f_uac1_legacy.c
> +++ b/drivers/usb/gadget/function/f_uac1_legacy.c
> @@ -11,6 +11,7 @@
>  #include <linux/module.h>
>  #include <linux/device.h>
>  #include <linux/atomic.h>
> +#include <asm/unaligned.h>
>  
>  #include "u_uac1_legacy.h"
>  
> @@ -370,9 +371,21 @@ static void f_audio_complete(struct usb_ep *ep, struct usb_request *req)
>  		if (ep == out_ep)
>  			f_audio_out_ep_complete(ep, req);
>  		else if (audio->set_con) {
> -			memcpy(&data, req->buf, req->length);
> -			audio->set_con->set(audio->set_con, audio->set_cmd,
> -					le16_to_cpu(data));
> +			struct usb_audio_control *con = audio->set_con;
> +
> +			if ((con->type == UAC_FU_MUTE && req->actual != sizeof(u8)) ||
> +				(con->type == UAC_FU_VOLUME && req->actual != sizeof(__le16)) ||
> +				(con->type != UAC_FU_MUTE && con->type != UAC_FU_VOLUME)) {
> +				usb_ep_set_halt(ep);
> +				audio->set_con = NULL;
> +				break;
> +			}
> +
> +			if (con->type == UAC_FU_MUTE)
> +				data = *(u8 *)req->buf;
> +			else
> +				data = get_unaligned_le16(req->buf);

Very cool, thanks for finding and fixing this.  But there is a bit of a
coding style issue in that big if check, AND it's hard to follow, how
about doing something like this instead (totally untested and not even
compiled:
			struct usb_audio_control *con = audio->set_con;
			u8 type = con->type;
			bool valid_request = false;

			switch (type) {
			case: UAC_FU_MUTE:
				if (req->actual == sizeof(__u8)) {
					memcpy(&data, req->buf, sizeof(__u8));
					valid_request = true;
				}
				break;
			case UAC_FU_VOLUME:
				if (req->actual == sizeof(__le16) {
					memcpy(&data, req->buf, sizeof(__le16));
					valid_request = true;
				}
				break;
			}
			if (valid_request)
				con->set(con, audio->set_cmd, data);
			else
				usb_ep_set_halt(ep);

  			audio->set_con = NULL;
		}

Does that make it easier to read?  I don't know, maybe not, your call,
but this way the memcpy of a constant size should be equalivant to your
direct cast if the compiler is sane, right?

And as this is a __le16 value, should we be doing some endian conversion
somewhere?

thanks,

greg k-h