diff mbox series

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

Message ID 20201002114323.GA3296553@kroah.com
State New
Headers show
Series [v2] net: hso: do not call unregister if not registered | expand

Commit Message

Greg Kroah-Hartman Oct. 2, 2020, 11:43 a.m. UTC 
From: Tuba Yavuz <tuba@ece.ufl.edu>

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 <tuba@ece.ufl.edu>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
v2: format cleaned up based on feedback from previous review
    Forward to Greg to submit due to email problems on Tuba's side

Comments

Salvatore Bonaccorso Aug. 16, 2021, 6:52 a.m. UTC | #1 
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 <gregkh@linuxfoundation.org>

> > 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 | #2 
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 <gregkh@linuxfoundation.org>

> > > 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 | #3 
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

________________________________________
From: Salvatore Bonaccorso <salvatore.bonaccorso@gmail.com> on behalf of Salvatore Bonaccorso <carnil@debian.org>

Sent: Monday, August 16, 2021 2:52 AM
To: Greg KH
Cc: David Miller; Yavuz, Tuba; netdev@vger.kernel.org; kuba@kernel.org; oneukum@suse.com; linux-kernel@vger.kernel.org; linux-usb@vger.kernel.org
Subject: Re: [PATCH v2] net: hso: do not call unregister if not registered

[External Email]

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 <gregkh@linuxfoundation.org>

> > 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
diff mbox series

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 */