usb: core: Strengthen error handling in hub_hub_status()

[PATCH] usb: core: Strengthen error handling in hub_hub_status()

Posted by Griffin Kroah-Hartman 1 week, 4 days ago

Add additional error handling after the call to get_hub_status() in
hub_hub_status().

get_hub_status() uses usb_control_msg() which does not verify that the
message is the correct length, substituting it for
usb_control_msg_recv() would also solve this issue but increase memory
allocations.

Instead, error handling is copied from the method used in
hub_ext_port_status(), which shares the same flow of logic as
hub_hub_status().

Assisted-by: gkh_clanker_t1000
Signed-off-by: Griffin Kroah-Hartman <griffin@kroah.com>
---
 drivers/usb/core/hub.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 5262e11c12cd..0d2166b8923a 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -991,10 +991,12 @@ static int hub_hub_status(struct usb_hub *hub,
 
 	mutex_lock(&hub->status_mutex);
 	ret = get_hub_status(hub->hdev, &hub->status->hub);
-	if (ret < 0) {
+	if (ret < sizeof(hub->status->hub)) {
 		if (ret != -ENODEV)
 			dev_err(hub->intfdev,
 				"%s failed (err = %d)\n", __func__, ret);
+		if (ret >= 0)
+			ret = -EIO;
 	} else {
 		*status = le16_to_cpu(hub->status->hub.wHubStatus);
 		*change = le16_to_cpu(hub->status->hub.wHubChange);

---
base-commit: a13c140cc289c0b7b3770bce5b3ad42ab35074aa
change-id: 20260714-usb_core_patches_2-a3eb9d1b7b9e

Best regards,
--  
Griffin Kroah-Hartman <griffin@kroah.com>

Re: [PATCH] usb: core: Strengthen error handling in hub_hub_status()

Posted by Alan Stern 1 week, 4 days ago

On Tue, Jul 14, 2026 at 11:04:33AM +0200, Griffin Kroah-Hartman wrote:
> Add additional error handling after the call to get_hub_status() in
> hub_hub_status().
> 
> get_hub_status() uses usb_control_msg() which does not verify that the
> message is the correct length, substituting it for
> usb_control_msg_recv() would also solve this issue but increase memory
> allocations.
> 
> Instead, error handling is copied from the method used in
> hub_ext_port_status(), which shares the same flow of logic as
> hub_hub_status().
> 
> Assisted-by: gkh_clanker_t1000
> Signed-off-by: Griffin Kroah-Hartman <griffin@kroah.com>
> ---
>  drivers/usb/core/hub.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 5262e11c12cd..0d2166b8923a 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -991,10 +991,12 @@ static int hub_hub_status(struct usb_hub *hub,
>  
>  	mutex_lock(&hub->status_mutex);
>  	ret = get_hub_status(hub->hdev, &hub->status->hub);
> -	if (ret < 0) {
> +	if (ret < sizeof(hub->status->hub)) {
>  		if (ret != -ENODEV)
>  			dev_err(hub->intfdev,
>  				"%s failed (err = %d)\n", __func__, ret);
> +		if (ret >= 0)
> +			ret = -EIO;

It would be better to put these two lines above the preceding test.  
That way it won't print confusing things like "hub_hub_status failed 
(err = 2)".

Alan Stern

>  	} else {
>  		*status = le16_to_cpu(hub->status->hub.wHubStatus);
>  		*change = le16_to_cpu(hub->status->hub.wHubChange);
> 
> ---
> base-commit: a13c140cc289c0b7b3770bce5b3ad42ab35074aa
> change-id: 20260714-usb_core_patches_2-a3eb9d1b7b9e
> 
> Best regards,
> --  
> Griffin Kroah-Hartman <griffin@kroah.com>

Re: [PATCH] usb: core: Strengthen error handling in hub_hub_status()

Posted by Greg Kroah-Hartman 1 week, 1 day ago

On Tue, Jul 14, 2026 at 12:32:06PM -0400, Alan Stern wrote:
> On Tue, Jul 14, 2026 at 11:04:33AM +0200, Griffin Kroah-Hartman wrote:
> > Add additional error handling after the call to get_hub_status() in
> > hub_hub_status().
> > 
> > get_hub_status() uses usb_control_msg() which does not verify that the
> > message is the correct length, substituting it for
> > usb_control_msg_recv() would also solve this issue but increase memory
> > allocations.
> > 
> > Instead, error handling is copied from the method used in
> > hub_ext_port_status(), which shares the same flow of logic as
> > hub_hub_status().
> > 
> > Assisted-by: gkh_clanker_t1000
> > Signed-off-by: Griffin Kroah-Hartman <griffin@kroah.com>
> > ---
> >  drivers/usb/core/hub.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index 5262e11c12cd..0d2166b8923a 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -991,10 +991,12 @@ static int hub_hub_status(struct usb_hub *hub,
> >  
> >  	mutex_lock(&hub->status_mutex);
> >  	ret = get_hub_status(hub->hdev, &hub->status->hub);
> > -	if (ret < 0) {
> > +	if (ret < sizeof(hub->status->hub)) {
> >  		if (ret != -ENODEV)
> >  			dev_err(hub->intfdev,
> >  				"%s failed (err = %d)\n", __func__, ret);
> > +		if (ret >= 0)
> > +			ret = -EIO;
> 
> It would be better to put these two lines above the preceding test.  
> That way it won't print confusing things like "hub_hub_status failed 
> (err = 2)".

It is confusing, but it matches the other check for this same error in
hub_ext_port_status() as it's the same error condition.  How about this
patch happens first int he series, which "unifies" them, and then fix up
both of the patterns at the same time to be more clear in a later patch
in the series?

thanks,

greg k-h

Re: [PATCH] usb: core: Strengthen error handling in hub_hub_status()

Posted by Alan Stern 1 week, 1 day ago

On Fri, Jul 17, 2026 at 11:48:46AM +0200, Greg Kroah-Hartman wrote:
> On Tue, Jul 14, 2026 at 12:32:06PM -0400, Alan Stern wrote:
> > On Tue, Jul 14, 2026 at 11:04:33AM +0200, Griffin Kroah-Hartman wrote:
> > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > > index 5262e11c12cd..0d2166b8923a 100644
> > > --- a/drivers/usb/core/hub.c
> > > +++ b/drivers/usb/core/hub.c
> > > @@ -991,10 +991,12 @@ static int hub_hub_status(struct usb_hub *hub,
> > >  
> > >  	mutex_lock(&hub->status_mutex);
> > >  	ret = get_hub_status(hub->hdev, &hub->status->hub);
> > > -	if (ret < 0) {
> > > +	if (ret < sizeof(hub->status->hub)) {
> > >  		if (ret != -ENODEV)
> > >  			dev_err(hub->intfdev,
> > >  				"%s failed (err = %d)\n", __func__, ret);
> > > +		if (ret >= 0)
> > > +			ret = -EIO;
> > 
> > It would be better to put these two lines above the preceding test.  
> > That way it won't print confusing things like "hub_hub_status failed 
> > (err = 2)".
> 
> It is confusing, but it matches the other check for this same error in
> hub_ext_port_status() as it's the same error condition.  How about this
> patch happens first int he series, which "unifies" them, and then fix up
> both of the patterns at the same time to be more clear in a later patch
> in the series?

That would be fine with me.

Alan Stern

Re: [PATCH] usb: core: Strengthen error handling in hub_hub_status()

Posted by Oliver Neukum 1 week, 4 days ago

On 14.07.26 18:32, Alan Stern wrote:

>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index 5262e11c12cd..0d2166b8923a 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -991,10 +991,12 @@ static int hub_hub_status(struct usb_hub *hub,
>>   
>>   	mutex_lock(&hub->status_mutex);
>>   	ret = get_hub_status(hub->hdev, &hub->status->hub);
>> -	if (ret < 0) {
>> +	if (ret < sizeof(hub->status->hub)) {
>>   		if (ret != -ENODEV)
>>   			dev_err(hub->intfdev,
>>   				"%s failed (err = %d)\n", __func__, ret);
>> +		if (ret >= 0)
>> +			ret = -EIO;
> 
> It would be better to put these two lines above the preceding test.
> That way it won't print confusing things like "hub_hub_status failed
> (err = 2)".
I am afraid I have to disagree. We would in effect say that get_hub_status()
has returned -EIO. That is simply not true.

	Regards
		Oliver

Re: [PATCH] usb: core: Strengthen error handling in hub_hub_status()

Posted by Alan Stern 1 week, 4 days ago

On Tue, Jul 14, 2026 at 07:48:14PM +0200, Oliver Neukum wrote:
> On 14.07.26 18:32, Alan Stern wrote:
> 
> > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > > index 5262e11c12cd..0d2166b8923a 100644
> > > --- a/drivers/usb/core/hub.c
> > > +++ b/drivers/usb/core/hub.c
> > > @@ -991,10 +991,12 @@ static int hub_hub_status(struct usb_hub *hub,
> > >   	mutex_lock(&hub->status_mutex);
> > >   	ret = get_hub_status(hub->hdev, &hub->status->hub);
> > > -	if (ret < 0) {
> > > +	if (ret < sizeof(hub->status->hub)) {
> > >   		if (ret != -ENODEV)
> > >   			dev_err(hub->intfdev,
> > >   				"%s failed (err = %d)\n", __func__, ret);
> > > +		if (ret >= 0)
> > > +			ret = -EIO;
> > 
> > It would be better to put these two lines above the preceding test.
> > That way it won't print confusing things like "hub_hub_status failed
> > (err = 2)".
> I am afraid I have to disagree. We would in effect say that get_hub_status()
> has returned -EIO. That is simply not true.

On the contrary: The message would say that hub_hub_status is returning 
-EIO, which _is_ true.

Of course, the error message could instead be changed in the way you 
suggest:

				dev_err(hub->intfev,
					"get_hub_status failed (%d)\n", ret);

Either my change or yours would be okay with me, but I don't like what 
the patch does.

(Another question is whether this message deserves to be dev_err().  
There isn't anything the user can do about it when it happens, so maybe 
it should be dev_dbg() instead.)

Alan Stern