usb: f_fs: Drop check on Reserved1 field on OS_DESC_EXT_COMPAT

PATCH In review John Keeping 2 participants 7 messages Nov 9, 2017 – Nov 27, 2017

Subsystems (2)

Stable branch USB

lore

Share & tools

lore

John Keeping

author

2017-11-09 16:34 UTC #0 lore raw

PATCH 1/1 usb: f_fs: Drop check on Reserved1 field on OS_DESC_EXT_COMPAT

This check has gone through several incompatible variations in commits 53642399aa71 ("usb: gadget: f_fs: Fix wrong check on reserved1 of OS_DESC_EXT_COMPAT"), 354bc45bf329 ("usb: gadget: f_fs: Fix ExtCompat descriptor validation") and 3ba534df815f ("Revert "usb: gadget: f_fs:
Fix ExtCompat descriptor validation"") after initially being introduced in commit f0175ab51993 ("usb: gadget: f_fs: OS descriptors support").

The various changes make it impossible for a single userspace implementation to work with different kernel versions, so let's just drop the condition to avoid breaking userspace.

Fixes 53642399aa71 ("usb: gadget: f_fs: Fix wrong check on reserved1 of OS_DESC_EXT_COMPAT")

Cc [email protected] # v4.7+

Signed-off-by John Keeping <[email protected]>

---
 drivers/usb/gadget/function/f_fs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/usb/gadget/function/f_fs.c +1 -2

index 652397eda6d6..0d9962834345 100644
@@ -2282,8 +2282,7 @@ 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;
 		for (i = 0; i < ARRAY_SIZE(d->Reserved2); ++i)
 			if (d->Reserved2[i])

Signature · 1 line

2.15.0

Felipe Balbi

2017-11-10 10:40 UTC #1 lore raw

Hi,

Quoted 23 lines · John Keeping

This check has gone through several incompatible variations in commits 53642399aa71 ("usb: gadget: f_fs: Fix wrong check on reserved1 of OS_DESC_EXT_COMPAT"), 354bc45bf329 ("usb: gadget: f_fs: Fix ExtCompat descriptor validation") and 3ba534df815f ("Revert "usb: gadget: f_fs:
Fix ExtCompat descriptor validation"") after initially being introduced in commit f0175ab51993 ("usb: gadget: f_fs: OS descriptors support").
The various changes make it impossible for a single userspace implementation to work with different kernel versions, so let's just drop the condition to avoid breaking userspace.
Fixes 53642399aa71 ("usb: gadget: f_fs: Fix wrong check on reserved1 of OS_DESC_EXT_COMPAT")
Cc [email protected] # v4.7+
Signed-off-by John Keeping <[email protected]>
---
 drivers/usb/gadget/function/f_fs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/usb/gadget/function/f_fs.c +0 -0
index 652397eda6d6..0d9962834345 100644

drivers/usb/gadget/function/f_fs.c static int __ffs_data_do_os_desc(enum ffs_os_desc_type type, · line 2288

Quoted 9 lines

drivers/usb/gadget/function/f_fs.c +1 -2

@@ -2282,8 +2282,7 @@ 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;
 		for (i = 0; i < ARRAY_SIZE(d->Reserved2); ++i)
 			if (d->Reserved2[i])

Sorry, but no. We want to be compliant with the specification. If there are older still-maintained stable trees which are not working, we need to backport a fix to them, but we're not allowing uncompliant implementations.

Signature · 1 line

balbi

John Keeping

author

2017-11-10 18:34 UTC #2 lore raw

Quoted 26 lines · Felipe Balbi

Quoted 25 lines · John Keeping
This check has gone through several incompatible variations in commits 53642399aa71 ("usb: gadget: f_fs: Fix wrong check on reserved1 of OS_DESC_EXT_COMPAT"), 354bc45bf329 ("usb: gadget: f_fs: Fix ExtCompat descriptor validation") and 3ba534df815f ("Revert "usb: gadget: f_fs:
Fix ExtCompat descriptor validation"") after initially being introduced in commit f0175ab51993 ("usb: gadget: f_fs: OS descriptors support").
The various changes make it impossible for a single userspace implementation to work with different kernel versions, so let's just drop the condition to avoid breaking userspace.
Fixes 53642399aa71 ("usb: gadget: f_fs: Fix wrong check on reserved1 of OS_DESC_EXT_COMPAT")
Cc [email protected] # v4.7+
Signed-off-by John Keeping <[email protected]>
---
 drivers/usb/gadget/function/f_fs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/usb/gadget/function/f_fs.c +0 -0
index 652397eda6d6..0d9962834345 100644
@@ -2282,8 +2282,7 @@ static int __ffs_data_do_os_desc(enum ffs_os_desc_type type,
 		int i;
 

Quoted 12 lines

drivers/usb/gadget/function/f_fs.c static int __ffs_data_do_os_desc(enum ffs_os_desc_type type, · line 2288
Quoted 7 lines
drivers/usb/gadget/function/f_fs.c +1 -2
 		if (len < sizeof(*d) ||
-		    d->bFirstInterfaceNumber >= ffs->interfaces_count ||
-		    !d->Reserved1)
+		    d->bFirstInterfaceNumber >= ffs->interfaces_count)
 			return -EINVAL;
 		for (i = 0; i < ARRAY_SIZE(d->Reserved2); ++i)
 			if (d->Reserved2[i])  
Sorry, but no. We want to be compliant with the specification. If there are older still-maintained stable trees which are not working, we need to backport a fix to them, but we're not allowing uncompliant implementations.

Aren't we allowing non-compliant implementations now? The spec says the value must be 1 but since v4.7 this code has allowed all non-zero values.

At this point I don't think the kernel can disallow any values of Reserved1 without breaking someone's userspace.

Felipe Balbi

2017-11-13 10:57 UTC #3 lore raw

Hi,

Quoted 41 lines · John Keeping

Quoted 26 lines · Felipe Balbi
Quoted 25 lines · John Keeping
This check has gone through several incompatible variations in commits 53642399aa71 ("usb: gadget: f_fs: Fix wrong check on reserved1 of OS_DESC_EXT_COMPAT"), 354bc45bf329 ("usb: gadget: f_fs: Fix ExtCompat descriptor validation") and 3ba534df815f ("Revert "usb: gadget: f_fs:
Fix ExtCompat descriptor validation"") after initially being introduced in commit f0175ab51993 ("usb: gadget: f_fs: OS descriptors support").
The various changes make it impossible for a single userspace implementation to work with different kernel versions, so let's just drop the condition to avoid breaking userspace.
Fixes 53642399aa71 ("usb: gadget: f_fs: Fix wrong check on reserved1 of OS_DESC_EXT_COMPAT")
Cc [email protected] # v4.7+
Signed-off-by John Keeping <[email protected]>
---
 drivers/usb/gadget/function/f_fs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/usb/gadget/function/f_fs.c +0 -0
index 652397eda6d6..0d9962834345 100644
@@ -2282,8 +2282,7 @@ static int __ffs_data_do_os_desc(enum ffs_os_desc_type type,
 		int i;
 
Quoted 12 lines
drivers/usb/gadget/function/f_fs.c static int __ffs_data_do_os_desc(enum ffs_os_desc_type type, · line 2288
Quoted 7 lines
drivers/usb/gadget/function/f_fs.c +1 -2
 		if (len < sizeof(*d) ||
-		    d->bFirstInterfaceNumber >= ffs->interfaces_count ||
-		    !d->Reserved1)
+		    d->bFirstInterfaceNumber >= ffs->interfaces_count)
 			return -EINVAL;
 		for (i = 0; i < ARRAY_SIZE(d->Reserved2); ++i)
 			if (d->Reserved2[i])  
Sorry, but no. We want to be compliant with the specification. If there are older still-maintained stable trees which are not working, we need to backport a fix to them, but we're not allowing uncompliant implementations.

Quoted 3 lines

Aren't we allowing non-compliant implementations now? The spec says the value must be 1 but since v4.7 this code has allowed all non-zero values.

Good point. Then how about we just force the value to 1 in f_fs.c and remove the check?

Signature · 1 line

balbi

John Keeping

author

2017-11-13 18:20 UTC #4 lore raw

Quoted 2 lines · Felipe Balbi

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 [email protected]

Signed-off-by John Keeping <[email protected]>

---
 drivers/usb/gadget/function/f_fs.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

drivers/usb/gadget/function/f_fs.c +11 -2

index 652397eda6d6..520a96e7ef9a 100644
@@ -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;
+		}
 		for (i = 0; i < ARRAY_SIZE(d->Reserved2); ++i)
 			if (d->Reserved2[i])
 				return -EINVAL;

Signature · 1 line

2.15.0

Felipe Balbi

2017-11-27 11:34 UTC #5 lore raw

Hi,

Quoted 43 lines · John Keeping

Quoted 2 lines · Felipe Balbi
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 [email protected]
Signed-off-by John Keeping <[email protected]>
---
 drivers/usb/gadget/function/f_fs.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
drivers/usb/gadget/function/f_fs.c +1 -2
index 652397eda6d6..520a96e7ef9a 100644
@@ -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;

drivers/usb/gadget/function/f_fs.c static int __ffs_data_do_os_desc(enum ffs_os_desc_type type, · line 2296

Quoted 10 lines

drivers/usb/gadget/function/f_fs.c +10 -0

+		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?

Signature · 1 line

balbi

John Keeping

author

2017-11-27 18:16 UTC #6 lore raw

PATCH v2 1/1 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.

Fixes 53642399aa71 ("usb: gadget: f_fs: Fix wrong check on reserved1 of OS_DESC_EXT_COMPAT")

Cc [email protected]

Signed-off-by John Keeping <[email protected]>

---
 drivers/usb/gadget/function/f_fs.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

Quoted 1 line · Felipe Balbi

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

Here it is.

drivers/usb/gadget/function/f_fs.c +11 -2

index 97ea059a7aa4..7537cc1a7d79 100644
@@ -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;
+		}
 		for (i = 0; i < ARRAY_SIZE(d->Reserved2); ++i)
 			if (d->Reserved2[i])
 				return -EINVAL;

Signature · 1 line

2.15.0