linux-kernel - Re: [PATCH] usb: f_fs: Drop check on Reserved1 field on OS_DESC_EXT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87vahwklab.fsf@linux.intel.com>
Date:   Mon, 27 Nov 2017 13:34:20 +0200
From:   Felipe Balbi <balbi@...nel.org>
To:     John Keeping <john@...anate.com>
Cc:     linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Vincent Pelletier <plr.vincent@...il.com>,
        Jim Lin <jilin@...dia.com>
Subject: Re: [PATCH] usb: f_fs: Drop check on Reserved1 field on OS_DESC_EXT_COMPAT


Hi,

John Keeping <john@...anate.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@...r.kernel.org
> Signed-off-by: John Keeping <john@...anate.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

Download attachment "signature.asc" of type "application/pgp-signature" (833 bytes)