Re: [PATCH] usb: f_fs: Drop check on Reserved1 field on OS_DESC_EXT_COMPAT

[Felipe Balbi <balbi@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2397 bytes --]


Hi,

John Keeping <john@metanate.com> writes:
> On Mon, 13 Nov 2017 12:57:21 +0200, Felipe Balbi wrote:
>> Good point. Then how about we just force the value to 1 in f_fs.c and
>> remove the check?
>
> That seems reasonable.  Something like this?
>
> -- >8 --
> Subject: [PATCH] usb: f_fs: Force Reserved1=1 in OS_DESC_EXT_COMPAT
>
> The specification says that the Reserved1 field in OS_DESC_EXT_COMPAT
> must have the value "1", but when this feature was first implemented we
> rejected any non-zero values.
>
> This was adjusted to accept all non-zero values (while now rejecting
> zero) in commit 53642399aa71 ("usb: gadget: f_fs: Fix wrong check on
> reserved1 of OS_DESC_EXT_COMPAT"), but that breaks any userspace
> programs that worked previously by returning EINVAL when Reserved1 == 0
> which was previously the only value that succeeded!
>
> If we just set the field to "1" ourselves, both old and new userspace
> programs continue to work correctly and, as a bonus, old programs are
> now compliant with the specification without having to fix anything
> themselves.
>
> Fixes: 53642399aa71 ("usb: gadget: f_fs: Fix wrong check on reserved1 of OS_DESC_EXT_COMPAT")
> Cc: stable@vger.kernel.org
> Signed-off-by: John Keeping <john@metanate.com>
> ---
>  drivers/usb/gadget/function/f_fs.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index 652397eda6d6..520a96e7ef9a 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -2282,9 +2282,18 @@ static int __ffs_data_do_os_desc(enum ffs_os_desc_type type,
>  		int i;
>  
>  		if (len < sizeof(*d) ||
> -		    d->bFirstInterfaceNumber >= ffs->interfaces_count ||
> -		    !d->Reserved1)
> +		    d->bFirstInterfaceNumber >= ffs->interfaces_count)
>  			return -EINVAL;
> +		if (d->Reserved1 != 1) {
> +			/*
> +			 * According to the spec, Reserved1 must be set to 1
> +			 * but older kernels incorrectly rejected non-zero
> +			 * values.  We fix it here to avoid returning EINVAL
> +			 * in response to values we used to accept.
> +			 */
> +			pr_debug("usb_ext_compat_desc::Reserved1 forced to 1\n");
> +			d->Reserved1 = 1;
> +		}

exactly like that. Care to send as a formal patch so I can apply?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]