[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-usb
Subject:    Re: [PATCH v3] usb: gadget: f_fs: Fix incorrect version checking of OS descs
From:       Andrzej Pietrasiewicz <andrzej.p () collabora ! com>
Date:       2023-02-28 10:32:57
Message-ID: 22883b1c-751d-f0fb-1529-227e5a96e66b () collabora ! com
[Download RAW message or body]

Hi,

W dniu 28.02.2023 o  09:56, Yuta Hayama pisze:
> Currently, the USB gadget framework supports only version 1.0 of the MS OS
> descriptor. OS desc has a field bcdVersion indicating its version, with
> v1.0 represented by the value 0x100. However, __ffs_do_os_desc_header()
> was expecting the incorrect value 0x1, so allow the correct value 0x100.
> 
> The bcdVersion field of the descriptor that is actually sent to the host
> is set by composite_setup() (in composite.c) to the fixed value 0x100.
> Therefore, it can be understood that __ffs_do_os_desc_header() is only
> performing a format check of the OS desc passed to functionfs. If a value
> other than 0x100 is accepted, there is no effect on communication over
> the USB bus. Indeed, until now __ffs_do_os_desc_header() has only accepted
> the incorrect value 0x1, but there was no problem with the communication
> over the USB bus.
> 
> However, this can be confusing for functionfs userspace drivers. Since
> bcdVersion=0x100 is used in actual communication, functionfs should accept
> the value 0x100.
> 
> Note that the correct value for bcdVersion in OS desc v1.0 is 0x100, but
> to avoid breaking old userspace drivers, the value 0x1 is also accepted as
> an exception. At this time, a message is output to notify the user to fix
> the userspace driver.
> 
> Signed-off-by: Yuta Hayama <hayama@lineo.co.jp>

Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>

> ---
> Changelog
> v3:
> - modify wording of pr_warn() message (thanks to Andrzej),
>    and remove unnecessary format specifier.
> 
> v2:
> - remove comma inserted incorrectly in pr_vdebug()
> - when bcd_version == 0x1, use pr_warn() instead of pr_vdebug()
> 
>   drivers/usb/gadget/function/f_fs.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index 8ad354741380..34ddb1802c5f 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -2292,8 +2292,11 @@ static int __ffs_do_os_desc_header(enum ffs_os_desc_type *next_type,
>   	u16 bcd_version = le16_to_cpu(desc->bcdVersion);
>   	u16 w_index = le16_to_cpu(desc->wIndex);
>   
> -	if (bcd_version != 1) {
> -		pr_vdebug("unsupported os descriptors version: %d",
> +	if (bcd_version == 0x1) {
> +		pr_warn("bcdVersion must be 0x0100, stored in Little Endian order. "
> +			"Userspace driver should be fixed, accepting 0x0001 for compatibility.\n");
> +	} else if (bcd_version != 0x100) {
> +		pr_vdebug("unsupported os descriptors version: 0x%x\n",
>   			  bcd_version);
>   		return -EINVAL;
>   	}

[prev in list] [next in list] [prev in thread] [next in thread]