usb: host: max3421: Fix shift-out-of-bounds in max3421_hub_control()

PATCH Superseded pip-izony 3 participants 13 messages May 17 – May 18 USB

lore

Share & tools

lore

pip-izony

author

2026-05-17 00:02 UTC #0 lore raw

PATCH 1/1 usb: host: max3421: Fix shift-out-of-bounds in max3421_hub_control()

From: Seungjin Bae <[email protected]>

So if a malicious userspace task with access to the root hub via /dev/bus/usb/.../001 issues a USBDEVFS_CONTROL ioctl with wValue greater than or equal to 32, the left shift operation invokes shift-out-of-bounds undefined behavior. This results in arbitrary bit corruption of port_status, including the normally-immutable change bits, which can bypass internal state checks and confuse the hub status.

Fix this by rejecting requests whose value exceeds the shift width before performing the shift.

Fixes 2d53139f3162 ("Add support for using a MAX3421E chip as a host driver.")

Signed-off-by Seungjin Bae <[email protected]>

---
 drivers/usb/host/max3421-hcd.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/usb/host/max3421-hcd.c +4 -0

index 0e17c988d36a..3d6b351dcb1a 100644
@@ -1694,6 +1694,8 @@ max3421_hub_control(struct usb_hcd *hcd, u16 type_req, u16 value, u16 index,
 						!pdata->vbus_active_level);
 			fallthrough;
 		default:
+			if (value >= 32)
+				goto error;
 			max3421_hcd->port_status &= ~(1 << value);
 		}
 		break;

@@ -1747,6 +1749,8 @@ max3421_hub_control(struct usb_hcd *hcd, u16 type_req, u16 value, u16 index,
 			max3421_reset_port(hcd);
 			fallthrough;
 		default:
+			if (value >= 32)
+				goto error;
 			if ((max3421_hcd->port_status & USB_PORT_STAT_POWER)
 			    != 0)
 				max3421_hcd->port_status |= (1 << value);

Signature · 1 line

2.43.0

pip-izony

author

2026-05-17 00:07 UTC #1 lore raw

PATCH v2 1/1 usb: host: max3421: Fix shift-out-of-bounds in max3421_hub_control()

From: Seungjin Bae <[email protected]>

The max3421_hub_control() function handles USB hub class requests to the virtual root hub. In the default branches of both the ClearPortFeature and SetPortFeature switch statements, it modifies max3421_hcd->port_status by left shifting 1 by the request's value parameter. However, it does not validate whether this shift will exceed the width of port_status.

Fix this by rejecting requests whose value exceeds the shift width before performing the shift.

Fixes 2d53139f3162 ("Add support for using a MAX3421E chip as a host driver.")

Signed-off-by Seungjin Bae <[email protected]>

---
v1 -> v2: Restored a paragraph in the commit message that was truncated.

 drivers/usb/host/max3421-hcd.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/usb/host/max3421-hcd.c +4 -0

index 0e17c988d36a..3d6b351dcb1a 100644
@@ -1694,6 +1694,8 @@ max3421_hub_control(struct usb_hcd *hcd, u16 type_req, u16 value, u16 index,
 						!pdata->vbus_active_level);
 			fallthrough;
 		default:
+			if (value >= 32)
+				goto error;
 			max3421_hcd->port_status &= ~(1 << value);
 		}
 		break;

@@ -1747,6 +1749,8 @@ max3421_hub_control(struct usb_hcd *hcd, u16 type_req, u16 value, u16 index,
 			max3421_reset_port(hcd);
 			fallthrough;
 		default:
+			if (value >= 32)
+				goto error;
 			if ((max3421_hcd->port_status & USB_PORT_STAT_POWER)
 			    != 0)
 				max3421_hcd->port_status |= (1 << value);

Signature · 1 line

2.43.0

Alan Stern

2026-05-17 01:15 UTC #2 lore raw

Quoted 17 lines · pip-izony

From: Seungjin Bae <[email protected]>
The max3421_hub_control() function handles USB hub class requests to the virtual root hub. In the default branches of both the ClearPortFeature and SetPortFeature switch statements, it modifies max3421_hcd->port_status by left shifting 1 by the request's value parameter. However, it does not validate whether this shift will exceed the width of port_status.
So if a malicious userspace task with access to the root hub via /dev/bus/usb/.../001 issues a USBDEVFS_CONTROL ioctl with wValue greater than or equal to 32, the left shift operation invokes shift-out-of-bounds undefined behavior. This results in arbitrary bit corruption of port_status, including the normally-immutable change bits, which can bypass internal state checks and confuse the hub status.

Quoted 2 lines

Fix this by rejecting requests whose value exceeds the shift width before performing the shift.

Another problem is that the root hub is supposed to reject requests to clear or set a feature for a non-existent port. Just as in the GetPortStatus case, the ClearPortFeature and SetPortFeature cases should check for index != 1.

Alan Stern

Seungjin Bae

author

2026-05-17 18:11 UTC #3 lore raw

2026년 5월 16일 (토) 오후 9:15, Alan Stern <[email protected]>님이 작성:

Quoted 22 lines · pip-izony

Quoted 17 lines
From: Seungjin Bae <[email protected]>
The max3421_hub_control() function handles USB hub class requests to the virtual root hub. In the default branches of both the ClearPortFeature and SetPortFeature switch statements, it modifies max3421_hcd->port_status by left shifting 1 by the request's value parameter. However, it does not validate whether this shift will exceed the width of port_status.
So if a malicious userspace task with access to the root hub via /dev/bus/usb/.../001 issues a USBDEVFS_CONTROL ioctl with wValue greater than or equal to 32, the left shift operation invokes shift-out-of-bounds undefined behavior. This results in arbitrary bit corruption of port_status, including the normally-immutable change bits, which can bypass internal state checks and confuse the hub status.
Quoted 2 lines
Fix this by rejecting requests whose value exceeds the shift width before performing the shift.

Quoted 6 lines

Another problem is that the root hub is supposed to reject requests to clear or set a feature for a non-existent port. Just as in the GetPortStatus case, the ClearPortFeature and SetPortFeature cases should check for index != 1.
Alan Stern

Good point.
I'll add the index != 1 check to both the ClearPortFeature and SetPortFeature cases in v3.
Thank you for the review.

Seungjin Bae

Greg Kroah-Hartman

maintainer

2026-05-17 05:49 UTC #4 lore raw

In reply to pip-izony

Quoted 24 lines · pip-izony

From: Seungjin Bae <[email protected]>
So if a malicious userspace task with access to the root hub via /dev/bus/usb/.../001 issues a USBDEVFS_CONTROL ioctl with wValue greater than or equal to 32, the left shift operation invokes shift-out-of-bounds undefined behavior. This results in arbitrary bit corruption of port_status, including the normally-immutable change bits, which can bypass internal state checks and confuse the hub status.
Fix this by rejecting requests whose value exceeds the shift width before performing the shift.
Fixes 2d53139f3162 ("Add support for using a MAX3421E chip as a host driver.")
Signed-off-by Seungjin Bae <[email protected]>
---
 drivers/usb/host/max3421-hcd.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/usb/host/max3421-hcd.c +0 -0
index 0e17c988d36a..3d6b351dcb1a 100644

drivers/usb/host/max3421-hcd.c max3421_hub_control(struct usb_hcd *hcd, u16 type_req, u16 value, u16 index, · line 1698

Quoted 5 lines

drivers/usb/host/max3421-hcd.c +2 -0

@@ -1694,6 +1694,8 @@ max3421_hub_control(struct usb_hcd *hcd, u16 type_req, u16 value, u16 index,
 						!pdata->vbus_active_level);
 			fallthrough;
 		default:
+			if (value >= 32)
+				goto error;

Cool, what tool found this? I've been running some static checkers and I don't think it turned this one up yet.

thanks,

greg k-h

Seungjin Bae

author

2026-05-17 18:19 UTC #5 lore raw

2026년 5월 17일 (일) 오전 1:49, Greg Kroah-Hartman <[email protected]>님이 작성:

Quoted 35 lines · pip-izony

Quoted 24 lines
From: Seungjin Bae <[email protected]>
So if a malicious userspace task with access to the root hub via /dev/bus/usb/.../001 issues a USBDEVFS_CONTROL ioctl with wValue greater than or equal to 32, the left shift operation invokes shift-out-of-bounds undefined behavior. This results in arbitrary bit corruption of port_status, including the normally-immutable change bits, which can bypass internal state checks and confuse the hub status.
Fix this by rejecting requests whose value exceeds the shift width before performing the shift.
Fixes 2d53139f3162 ("Add support for using a MAX3421E chip as a host driver.")
Signed-off-by Seungjin Bae <[email protected]>
---
 drivers/usb/host/max3421-hcd.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/usb/host/max3421-hcd.c +0 -0
index 0e17c988d36a..3d6b351dcb1a 100644
drivers/usb/host/max3421-hcd.c max3421_hub_control(struct usb_hcd *hcd, u16 type_req, u16 value, u16 index, · line 1698
Quoted 5 lines
drivers/usb/host/max3421-hcd.c +2 -0
@@ -1694,6 +1694,8 @@ max3421_hub_control(struct usb_hcd *hcd, u16 type_req, u16 value, u16 index,
                                              !pdata->vbus_active_level);
                      fallthrough;
              default:
+                     if (value >= 32)
+                             goto error;
Cool, what tool found this? I've been running some static checkers and I don't think it turned this one up yet.

Quoted 3 lines

thanks,
greg k-h

Thanks for your interest!

It's a KLEE-based symbolic execution tool I've been developing for kernel drivers. It's still a work in progress, but I'd be happy to share more details and the tool itself once it's in better shape.

Seungjin Bae

Greg Kroah-Hartman

maintainer

2026-05-18 05:19 UTC #6 lore raw

Quoted 38 lines · Seungjin Bae

2026년 5월 17일 (일) 오전 1:49, Greg Kroah-Hartman <[email protected]>님이 작성:
Quoted 37 lines · pip-izony
Quoted 24 lines
From: Seungjin Bae <[email protected]>
So if a malicious userspace task with access to the root hub via /dev/bus/usb/.../001 issues a USBDEVFS_CONTROL ioctl with wValue greater than or equal to 32, the left shift operation invokes shift-out-of-bounds undefined behavior. This results in arbitrary bit corruption of port_status, including the normally-immutable change bits, which can bypass internal state checks and confuse the hub status.
Fix this by rejecting requests whose value exceeds the shift width before performing the shift.
Fixes 2d53139f3162 ("Add support for using a MAX3421E chip as a host driver.")
Signed-off-by Seungjin Bae <[email protected]>
---
 drivers/usb/host/max3421-hcd.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/usb/host/max3421-hcd.c +0 -0
index 0e17c988d36a..3d6b351dcb1a 100644
drivers/usb/host/max3421-hcd.c max3421_hub_control(struct usb_hcd *hcd, u16 type_req, u16 value, u16 index, · line 1698
Quoted 5 lines
drivers/usb/host/max3421-hcd.c +2 -0
@@ -1694,6 +1694,8 @@ max3421_hub_control(struct usb_hcd *hcd, u16 type_req, u16 value, u16 index,
                                              !pdata->vbus_active_level);
                      fallthrough;
              default:
+                     if (value >= 32)
+                             goto error;
Cool, what tool found this? I've been running some static checkers and I don't think it turned this one up yet.
thanks,

Quoted 7 lines

Quoted 1 line
greg k-h
Thanks for your interest!
It's a KLEE-based symbolic execution tool I've been developing for kernel drivers. It's still a work in progress, but I'd be happy to share more details and the tool itself once it's in better shape.

As per our documentation, you MUST document the fact that you are using a tool to find/fix things. Please fix up our newly submitted patches to include that information.

thanks,

greg k-h

pip-izony

author

2026-05-17 19:03 UTC #7 lore raw

PATCH v3 1/2 usb: host: max3421: Fix shift-out-of-bounds in max3421_hub_control()

In reply to pip-izony

From: Seungjin Bae <[email protected]>

Fix this by rejecting requests whose value exceeds the shift width before performing the shift.

Fixes 2d53139f3162 ("Add support for using a MAX3421E chip as a host driver.")

Signed-off-by Seungjin Bae <[email protected]>

---
v1 -> v2: Restored a paragraph in the commit message that was truncated. v2 -> v3: No functional changes; reorganized as part of a 2-patch series.

 drivers/usb/host/max3421-hcd.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/usb/host/max3421-hcd.c +4 -0

index 0e17c988d36a..3d6b351dcb1a 100644
@@ -1694,6 +1694,8 @@ max3421_hub_control(struct usb_hcd *hcd, u16 type_req, u16 value, u16 index,
 						!pdata->vbus_active_level);
 			fallthrough;
 		default:
+			if (value >= 32)
+				goto error;
 			max3421_hcd->port_status &= ~(1 << value);
 		}
 		break;

@@ -1747,6 +1749,8 @@ max3421_hub_control(struct usb_hcd *hcd, u16 type_req, u16 value, u16 index,
 			max3421_reset_port(hcd);
 			fallthrough;
 		default:
+			if (value >= 32)
+				goto error;
 			if ((max3421_hcd->port_status & USB_PORT_STAT_POWER)
 			    != 0)
 				max3421_hcd->port_status |= (1 << value);

Signature · 1 line

2.43.0

pip-izony

author

2026-05-17 19:03 UTC #8 lore raw

PATCH v3 2/2 usb: host: max3421: Reject hub port requests for non-existent ports

From: Seungjin Bae <[email protected]>

The max3421_hub_control() function handles USB hub class requests to the virtual root hub. The GetPortStatus case correctly rejects requests with index != 1, since the virtual root hub has only a single port. However, the ClearPortFeature and SetPortFeature cases lack the same check.

Fix this by extending the index != 1 rejection to both cases, matching the existing behavior of GetPortStatus.

Fixes 2d53139f3162 ("Add support for using a MAX3421E chip as a host driver.")

Signed-off-by Seungjin Bae <[email protected]>

---
v3: New patch in v3, suggested by Alan Stern.

 drivers/usb/host/max3421-hcd.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/usb/host/max3421-hcd.c +4 -0

index 3d6b351dcb1a..73e76d0e6973 100644
@@ -1685,6 +1685,8 @@ max3421_hub_control(struct usb_hcd *hcd, u16 type_req, u16 value, u16 index,
 	case ClearHubFeature:
 		break;
 	case ClearPortFeature:
+		if (index != 1)
+			goto error;
 		switch (value) {
 		case USB_PORT_FEAT_SUSPEND:
 			break;

@@ -1728,6 +1730,8 @@ max3421_hub_control(struct usb_hcd *hcd, u16 type_req, u16 value, u16 index,
 		break;
 
 	case SetPortFeature:
+		if (index != 1)
+			goto error;
 		switch (value) {
 		case USB_PORT_FEAT_LINK_STATE:
 		case USB_PORT_FEAT_U1_TIMEOUT:

Signature · 1 line

2.43.0

Alan Stern

Reviewed-by on v3 2/2

2026-05-18 21:54 UTC #9 lore raw

Re: [PATCH v3 2/2] usb: host: max3421: Reject hub port requests for non-existent ports

Quoted 8 lines · pip-izony

From: Seungjin Bae <[email protected]>
The max3421_hub_control() function handles USB hub class requests to the virtual root hub. The GetPortStatus case correctly rejects requests with index != 1, since the virtual root hub has only a single port. However, the ClearPortFeature and SetPortFeature cases lack the same check.

Quoted 7 lines

Fix this by extending the index != 1 rejection to both cases, matching the existing behavior of GetPortStatus.
Fixes 2d53139f3162 ("Add support for using a MAX3421E chip as a host driver.")
Suggested-by Alan Stern <[email protected]>
Signed-off-by Seungjin Bae <[email protected]>
---

Reviewed-by Alan Stern <[email protected]>

Heh, this made me go back and look at dummy-hcd.c. It's missing the same check in the same places!

Would you like to submit a patch to fix that driver as well? If not, I'll take care of it.

Alan Stern

Quoted 30 lines

v3: New patch in v3, suggested by Alan Stern.

 drivers/usb/host/max3421-hcd.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/usb/host/max3421-hcd.c +4 -0

index 3d6b351dcb1a..73e76d0e6973 100644
@@ -1685,6 +1685,8 @@ max3421_hub_control(struct usb_hcd *hcd, u16 type_req, u16 value, u16 index,
 	case ClearHubFeature:
 		break;
 	case ClearPortFeature:
+		if (index != 1)
+			goto error;
 		switch (value) {
 		case USB_PORT_FEAT_SUSPEND:
 			break;

@@ -1728,6 +1730,8 @@ max3421_hub_control(struct usb_hcd *hcd, u16 type_req, u16 value, u16 index,
 		break;
 
 	case SetPortFeature:
+		if (index != 1)
+			goto error;
 		switch (value) {
 		case USB_PORT_FEAT_LINK_STATE:
 		case USB_PORT_FEAT_U1_TIMEOUT:

Signature · 1 line

2.43.0

Seungjin Bae

author

2026-05-18 22:37 UTC #10 lore raw

Re: [PATCH v3 2/2] usb: host: max3421: Reject hub port requests for non-existent ports

2026년 5월 18일 (월) 오후 5:54, Alan Stern <[email protected]>님이 작성:

Quoted 18 lines · pip-izony

Quoted 8 lines
From: Seungjin Bae <[email protected]>
The max3421_hub_control() function handles USB hub class requests to the virtual root hub. The GetPortStatus case correctly rejects requests with index != 1, since the virtual root hub has only a single port. However, the ClearPortFeature and SetPortFeature cases lack the same check.
Quoted 7 lines
Fix this by extending the index != 1 rejection to both cases, matching the existing behavior of GetPortStatus.
Fixes 2d53139f3162 ("Add support for using a MAX3421E chip as a host driver.")
Suggested-by Alan Stern <[email protected]>
Signed-off-by Seungjin Bae <[email protected]>
---

Quoted 7 lines

Reviewed-by Alan Stern <[email protected]>
Heh, this made me go back and look at dummy-hcd.c. It's missing the same check in the same places!
Would you like to submit a patch to fix that driver as well? If not, I'll take care of it.

Thanks for pointing this out.
I'll send a separate patch for that shortly.

Seungjin Bae

Quoted 33 lines

Alan Stern

Quoted 30 lines

v3: New patch in v3, suggested by Alan Stern.

 drivers/usb/host/max3421-hcd.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/usb/host/max3421-hcd.c +4 -1

index 3d6b351dcb1a..73e76d0e6973 100644
@@ -1685,6 +1685,8 @@ max3421_hub_control(struct usb_hcd *hcd, u16 type_req, u16 value, u16 index,
      case ClearHubFeature:
              break;
      case ClearPortFeature:
+             if (index != 1)
+                     goto error;
              switch (value) {
              case USB_PORT_FEAT_SUSPEND:
                      break;

@@ -1728,6 +1730,8 @@ max3421_hub_control(struct usb_hcd *hcd, u16 type_req, u16 value, u16 index,
              break;
 
      case SetPortFeature:
+             if (index != 1)
+                     goto error;
              switch (value) {
              case USB_PORT_FEAT_LINK_STATE:
              case USB_PORT_FEAT_U1_TIMEOUT:
--

2.43.0

pip-izony

author

2026-05-18 22:49 UTC #11 lore raw

PATCH v4 1/2 usb: host: max3421: Fix shift-out-of-bounds in max3421_hub_control()

In reply to pip-izony

From: Seungjin Bae <[email protected]>

Fix this by rejecting requests whose value exceeds the shift width before performing the shift.

This issue was found using a KLEE-based symbolic execution tool for kernel drivers that I'm currently developing.

Fixes 2d53139f3162 ("Add support for using a MAX3421E chip as a host driver.")

Signed-off-by Seungjin Bae <[email protected]>

---
v1 -> v2: Restored a paragraph in the commit message that was truncated. v2 -> v3: No functional changes; reorganized as part of a 2-patch series. v3 -> v4: Document the tool used to find this issue.

 drivers/usb/host/max3421-hcd.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/usb/host/max3421-hcd.c +4 -0

index 0e17c988d36a..3d6b351dcb1a 100644
@@ -1694,6 +1694,8 @@ max3421_hub_control(struct usb_hcd *hcd, u16 type_req, u16 value, u16 index,
 						!pdata->vbus_active_level);
 			fallthrough;
 		default:
+			if (value >= 32)
+				goto error;
 			max3421_hcd->port_status &= ~(1 << value);
 		}
 		break;

@@ -1747,6 +1749,8 @@ max3421_hub_control(struct usb_hcd *hcd, u16 type_req, u16 value, u16 index,
 			max3421_reset_port(hcd);
 			fallthrough;
 		default:
+			if (value >= 32)
+				goto error;
 			if ((max3421_hcd->port_status & USB_PORT_STAT_POWER)
 			    != 0)
 				max3421_hcd->port_status |= (1 << value);

Signature · 1 line

2.43.0

pip-izony

author

2026-05-18 22:49 UTC #12 lore raw

PATCH v4 2/2 usb: host: max3421: Reject hub port requests for non-existent ports

From: Seungjin Bae <[email protected]>

Fix this by extending the index != 1 rejection to both cases, matching the existing behavior of GetPortStatus.

Fixes 2d53139f3162 ("Add support for using a MAX3421E chip as a host driver.")

Reviewed-by Alan Stern <[email protected]>

Signed-off-by Seungjin Bae <[email protected]>

---
v3: New patch in v3, suggested by Alan Stern.
v3 -> v4: No functional changes.

 drivers/usb/host/max3421-hcd.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/usb/host/max3421-hcd.c +4 -0

index 3d6b351dcb1a..73e76d0e6973 100644
@@ -1685,6 +1685,8 @@ max3421_hub_control(struct usb_hcd *hcd, u16 type_req, u16 value, u16 index,
 	case ClearHubFeature:
 		break;
 	case ClearPortFeature:
+		if (index != 1)
+			goto error;
 		switch (value) {
 		case USB_PORT_FEAT_SUSPEND:
 			break;

@@ -1728,6 +1730,8 @@ max3421_hub_control(struct usb_hcd *hcd, u16 type_req, u16 value, u16 index,
 		break;
 
 	case SetPortFeature:
+		if (index != 1)
+			goto error;
 		switch (value) {
 		case USB_PORT_FEAT_LINK_STATE:
 		case USB_PORT_FEAT_U1_TIMEOUT:

Signature · 1 line

2.43.0