From: Tuba Yavuz <[email protected]>
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 <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
v2: format cleaned up based on feedback from previous review
Forward to Greg to submit due to email problems on Tuba's side
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 */
From: Greg KH <[email protected]>
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.
On Sat, Oct 03, 2020 at 05:00:42PM -0700, David Miller wrote:
> From: Greg KH <[email protected]>
> 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
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 <[email protected]>
> > 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
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 <[email protected]>
> > > 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 :(
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 <[email protected]> on behalf of Salvatore Bonaccorso <[email protected]>
Sent: Monday, August 16, 2021 2:52 AM
To: Greg KH
Cc: David Miller; Yavuz, Tuba; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
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 <[email protected]>
> > 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