[v2] net: hso: do not call unregister if not registered

Commit Message

Greg Kroah-Hartman Oct. 2, 2020, 11:43 a.m. UTC

From: Tuba Yavuz <[e-mail]>

On an error path inside the hso_create_net_device function of the hso
driver, hso_free_net_device gets called. This causes a use-after-free
and a double-free if register_netdev has not been called yet as
hso_free_net_device calls unregister_netdev regardless. I think the
driver should distinguish these cases and call unregister_netdev only if
register_netdev has been called.

Signed-off-by: Tuba Yavuz <[e-mail]>
Signed-off-by: Greg Kroah-Hartman <[e-mail]>
---
v2: format cleaned up based on feedback from previous review
    Forward to Greg to submit due to email problems on Tuba's side

Comments

David Miller Oct. 4, 2020, midnight UTC | #1

From: Greg KH <[e-mail]>
Date: Fri, 2 Oct 2020 13:43:23 +0200

> @@ -2366,7 +2366,8 @@ static void hso_free_net_device(struct hso_device *hso_dev, bool bailout)
>  
>  	remove_net_device(hso_net->parent);
>  
> -	if (hso_net->net)
> +	if (hso_net->net &&
> +	    hso_net->net->reg_state == NETREG_REGISTERED)
>  		unregister_netdev(hso_net->net);
>  
>  	/* start freeing */

I really want to get out of the habit of drivers testing the internal
netdev registration state to make decisions.

Instead, please track this internally.  You know if you registered the
device or not, therefore use that to control whether you try to
unregister it or not.

Thank you.

Greg Kroah-Hartman Oct. 4, 2020, 7:14 a.m. UTC | #2

On Sat, Oct 03, 2020 at 05:00:42PM -0700, David Miller wrote:
> From: Greg KH <[e-mail]>
> Date: Fri, 2 Oct 2020 13:43:23 +0200
> 
> > @@ -2366,7 +2366,8 @@ static void hso_free_net_device(struct hso_device *hso_dev, bool bailout)
> >  
> >  	remove_net_device(hso_net->parent);
> >  
> > -	if (hso_net->net)
> > +	if (hso_net->net &&
> > +	    hso_net->net->reg_state == NETREG_REGISTERED)
> >  		unregister_netdev(hso_net->net);
> >  
> >  	/* start freeing */
> 
> I really want to get out of the habit of drivers testing the internal
> netdev registration state to make decisions.
> 
> Instead, please track this internally.  You know if you registered the
> device or not, therefore use that to control whether you try to
> unregister it or not.

Fair enough.  Tuba, do you want to fix this up in this way, or do you
recommend that someone else do it?

thanks,

greg k-h

Salvatore Bonaccorso Aug. 16, 2021, 6:52 a.m. UTC | #3

Hi Greg, Tuba,

On Sun, Oct 04, 2020 at 09:14:33AM +0200, Greg KH wrote:
> On Sat, Oct 03, 2020 at 05:00:42PM -0700, David Miller wrote:
> > From: Greg KH <[e-mail]>
> > Date: Fri, 2 Oct 2020 13:43:23 +0200
> > 
> > > @@ -2366,7 +2366,8 @@ static void hso_free_net_device(struct hso_device *hso_dev, bool bailout)
> > >  
> > >  	remove_net_device(hso_net->parent);
> > >  
> > > -	if (hso_net->net)
> > > +	if (hso_net->net &&
> > > +	    hso_net->net->reg_state == NETREG_REGISTERED)
> > >  		unregister_netdev(hso_net->net);
> > >  
> > >  	/* start freeing */
> > 
> > I really want to get out of the habit of drivers testing the internal
> > netdev registration state to make decisions.
> > 
> > Instead, please track this internally.  You know if you registered the
> > device or not, therefore use that to control whether you try to
> > unregister it or not.
> 
> Fair enough.  Tuba, do you want to fix this up in this way, or do you
> recommend that someone else do it?

Do I miss something, or did that possibly fall through the cracks?

I was checking some open issues on a downstream distro side and found
htat this thread did not got a follow-up.

Regards,
Salvatore

Greg Kroah-Hartman Aug. 16, 2021, 7:02 a.m. UTC | #4

On Mon, Aug 16, 2021 at 08:52:58AM +0200, Salvatore Bonaccorso wrote:
> Hi Greg, Tuba,
> 
> On Sun, Oct 04, 2020 at 09:14:33AM +0200, Greg KH wrote:
> > On Sat, Oct 03, 2020 at 05:00:42PM -0700, David Miller wrote:
> > > From: Greg KH <[e-mail]>
> > > Date: Fri, 2 Oct 2020 13:43:23 +0200
> > > 
> > > > @@ -2366,7 +2366,8 @@ static void hso_free_net_device(struct hso_device *hso_dev, bool bailout)
> > > >  
> > > >  	remove_net_device(hso_net->parent);
> > > >  
> > > > -	if (hso_net->net)
> > > > +	if (hso_net->net &&
> > > > +	    hso_net->net->reg_state == NETREG_REGISTERED)
> > > >  		unregister_netdev(hso_net->net);
> > > >  
> > > >  	/* start freeing */
> > > 
> > > I really want to get out of the habit of drivers testing the internal
> > > netdev registration state to make decisions.
> > > 
> > > Instead, please track this internally.  You know if you registered the
> > > device or not, therefore use that to control whether you try to
> > > unregister it or not.
> > 
> > Fair enough.  Tuba, do you want to fix this up in this way, or do you
> > recommend that someone else do it?
> 
> Do I miss something, or did that possibly fall through the cracks?
> 
> I was checking some open issues on a downstream distro side and found
> htat this thread did not got a follow-up.

I did not see a follow-up patch :(

Yavuz, Tuba Aug. 17, 2021, 3:19 p.m. UTC | #5

Hi Salvatore,

I think it would be best if one of the developers of the hso driver could develop a patch along the lines of David Miller's earlier suggestion.

Best,

Tuba

Patch

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 2bb28db89432..e6b56bdf691d 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -2366,7 +2366,8 @@  static void hso_free_net_device(struct hso_device *hso_dev, bool bailout)
 
 	remove_net_device(hso_net->parent);
 
-	if (hso_net->net)
+	if (hso_net->net &&
+	    hso_net->net->reg_state == NETREG_REGISTERED)
 		unregister_netdev(hso_net->net);
 
 	/* start freeing */