[PATCH 5.4 0/3] usb: hso: backport CVE-2021-37159 fix

[PATCH 5.4 0/3] usb: hso: backport CVE-2021-37159 fix

[Ovidiu Panait]

All 3 upstream commits apply cleanly:
   * 5fcfb6d0bfcd ("hso: fix bailout in error case of probe") is a support
     patch needed for context
   * a6ecfb39ba9d ("usb: hso: fix error handling code of hso_create_net_device")
     is the actual fix
   * dcb713d53e2e ("usb: hso: remove the bailout parameter") is a follow up
     cleanup commit

Dongliang Mu (2):
  usb: hso: fix error handling code of hso_create_net_device
  usb: hso: remove the bailout parameter

Oliver Neukum (1):
  hso: fix bailout in error case of probe

 drivers/net/usb/hso.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

-- 
2.25.1

[PATCH 5.4 1/3] hso: fix bailout in error case of probe

[Ovidiu Panait]

From: Oliver Neukum <oneukum@suse.com>

commit 5fcfb6d0bfcda17f0d0656e4e5b3710af2bbaae5 upstream.

The driver tries to reuse code for disconnect in case
of a failed probe.
If resources need to be freed after an error in probe, the
netdev must not be freed because it has never been registered.
Fix it by telling the helper which path we are in.

Signed-off-by: Oliver Neukum <oneukum@suse.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Ovidiu Panait <ovidiu.panait@windriver.com>
---
 drivers/net/usb/hso.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 22450c4a9225..fb8827dd5671 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -2354,7 +2354,7 @@ static int remove_net_device(struct hso_device *hso_dev)
 }
 
 /* Frees our network device */
-static void hso_free_net_device(struct hso_device *hso_dev)
+static void hso_free_net_device(struct hso_device *hso_dev, bool bailout)
 {
 	int i;
 	struct hso_net *hso_net = dev2net(hso_dev);
@@ -2377,7 +2377,7 @@ static void hso_free_net_device(struct hso_device *hso_dev)
 	kfree(hso_net->mux_bulk_tx_buf);
 	hso_net->mux_bulk_tx_buf = NULL;
 
-	if (hso_net->net)
+	if (hso_net->net && !bailout)
 		free_netdev(hso_net->net);
 
 	kfree(hso_dev);
@@ -2553,7 +2553,7 @@ static struct hso_device *hso_create_net_device(struct usb_interface *interface,
 
 	return hso_dev;
 exit:
-	hso_free_net_device(hso_dev);
+	hso_free_net_device(hso_dev, true);
 	return NULL;
 }
 
@@ -3122,7 +3122,7 @@ static void hso_free_interface(struct usb_interface *interface)
 				rfkill_unregister(rfk);
 				rfkill_destroy(rfk);
 			}
-			hso_free_net_device(network_table[i]);
+			hso_free_net_device(network_table[i], false);
 		}
 	}
 }
-- 
2.25.1

[PATCH 5.4 2/3] usb: hso: fix error handling code of hso_create_net_device

[Ovidiu Panait]

From: Dongliang Mu <mudongliangabcd@gmail.com>

commit a6ecfb39ba9d7316057cea823b196b734f6b18ca upstream.

The current error handling code of hso_create_net_device is
hso_free_net_device, no matter which errors lead to. For example,
WARNING in hso_free_net_device [1].

Fix this by refactoring the error handling code of
hso_create_net_device by handling different errors by different code.

[1] https://syzkaller.appspot.com/bug?id=66eff8d49af1b28370ad342787413e35bbe76efe

Reported-by: syzbot+44d53c7255bb1aea22d2@syzkaller.appspotmail.com
Fixes: 5fcfb6d0bfcd ("hso: fix bailout in error case of probe")
Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Ovidiu Panait <ovidiu.panait@windriver.com>
---
 drivers/net/usb/hso.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index fb8827dd5671..762f7a4db809 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -2497,7 +2497,7 @@ static struct hso_device *hso_create_net_device(struct usb_interface *interface,
 			   hso_net_init);
 	if (!net) {
 		dev_err(&interface->dev, "Unable to create ethernet device\n");
-		goto exit;
+		goto err_hso_dev;
 	}
 
 	hso_net = netdev_priv(net);
@@ -2510,13 +2510,13 @@ static struct hso_device *hso_create_net_device(struct usb_interface *interface,
 				      USB_DIR_IN);
 	if (!hso_net->in_endp) {
 		dev_err(&interface->dev, "Can't find BULK IN endpoint\n");
-		goto exit;
+		goto err_net;
 	}
 	hso_net->out_endp = hso_get_ep(interface, USB_ENDPOINT_XFER_BULK,
 				       USB_DIR_OUT);
 	if (!hso_net->out_endp) {
 		dev_err(&interface->dev, "Can't find BULK OUT endpoint\n");
-		goto exit;
+		goto err_net;
 	}
 	SET_NETDEV_DEV(net, &interface->dev);
 	SET_NETDEV_DEVTYPE(net, &hso_type);
@@ -2525,18 +2525,18 @@ static struct hso_device *hso_create_net_device(struct usb_interface *interface,
 	for (i = 0; i < MUX_BULK_RX_BUF_COUNT; i++) {
 		hso_net->mux_bulk_rx_urb_pool[i] = usb_alloc_urb(0, GFP_KERNEL);
 		if (!hso_net->mux_bulk_rx_urb_pool[i])
-			goto exit;
+			goto err_mux_bulk_rx;
 		hso_net->mux_bulk_rx_buf_pool[i] = kzalloc(MUX_BULK_RX_BUF_SIZE,
 							   GFP_KERNEL);
 		if (!hso_net->mux_bulk_rx_buf_pool[i])
-			goto exit;
+			goto err_mux_bulk_rx;
 	}
 	hso_net->mux_bulk_tx_urb = usb_alloc_urb(0, GFP_KERNEL);
 	if (!hso_net->mux_bulk_tx_urb)
-		goto exit;
+		goto err_mux_bulk_rx;
 	hso_net->mux_bulk_tx_buf = kzalloc(MUX_BULK_TX_BUF_SIZE, GFP_KERNEL);
 	if (!hso_net->mux_bulk_tx_buf)
-		goto exit;
+		goto err_free_tx_urb;
 
 	add_net_device(hso_dev);
 
@@ -2544,7 +2544,7 @@ static struct hso_device *hso_create_net_device(struct usb_interface *interface,
 	result = register_netdev(net);
 	if (result) {
 		dev_err(&interface->dev, "Failed to register device\n");
-		goto exit;
+		goto err_free_tx_buf;
 	}
 
 	hso_log_port(hso_dev);
@@ -2552,8 +2552,21 @@ static struct hso_device *hso_create_net_device(struct usb_interface *interface,
 	hso_create_rfkill(hso_dev, interface);
 
 	return hso_dev;
-exit:
-	hso_free_net_device(hso_dev, true);
+
+err_free_tx_buf:
+	remove_net_device(hso_dev);
+	kfree(hso_net->mux_bulk_tx_buf);
+err_free_tx_urb:
+	usb_free_urb(hso_net->mux_bulk_tx_urb);
+err_mux_bulk_rx:
+	for (i = 0; i < MUX_BULK_RX_BUF_COUNT; i++) {
+		usb_free_urb(hso_net->mux_bulk_rx_urb_pool[i]);
+		kfree(hso_net->mux_bulk_rx_buf_pool[i]);
+	}
+err_net:
+	free_netdev(net);
+err_hso_dev:
+	kfree(hso_dev);
 	return NULL;
 }
 
-- 
2.25.1

[PATCH 5.4 3/3] usb: hso: remove the bailout parameter

[Ovidiu Panait]

From: Dongliang Mu <mudongliangabcd@gmail.com>

commit dcb713d53e2eadf42b878c12a471e74dc6ed3145 upstream.

There are two invocation sites of hso_free_net_device. After
refactoring hso_create_net_device, this parameter is useless.
Remove the bailout in the hso_free_net_device and change the invocation
sites of this function.

Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Ovidiu Panait <ovidiu.panait@windriver.com>
---
 drivers/net/usb/hso.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 762f7a4db809..95da2576a221 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -2354,7 +2354,7 @@ static int remove_net_device(struct hso_device *hso_dev)
 }
 
 /* Frees our network device */
-static void hso_free_net_device(struct hso_device *hso_dev, bool bailout)
+static void hso_free_net_device(struct hso_device *hso_dev)
 {
 	int i;
 	struct hso_net *hso_net = dev2net(hso_dev);
@@ -2377,7 +2377,7 @@ static void hso_free_net_device(struct hso_device *hso_dev, bool bailout)
 	kfree(hso_net->mux_bulk_tx_buf);
 	hso_net->mux_bulk_tx_buf = NULL;
 
-	if (hso_net->net && !bailout)
+	if (hso_net->net)
 		free_netdev(hso_net->net);
 
 	kfree(hso_dev);
@@ -3135,7 +3135,7 @@ static void hso_free_interface(struct usb_interface *interface)
 				rfkill_unregister(rfk);
 				rfkill_destroy(rfk);
 			}
-			hso_free_net_device(network_table[i], false);
+			hso_free_net_device(network_table[i]);
 		}
 	}
 }
-- 
2.25.1

Re: [PATCH 5.4 0/3] usb: hso: backport CVE-2021-37159 fix

[Salvatore Bonaccorso]

Hi Ovidiu

On Tue, Sep 28, 2021 at 04:15:20PM +0300, Ovidiu Panait wrote:
> All 3 upstream commits apply cleanly:
>    * 5fcfb6d0bfcd ("hso: fix bailout in error case of probe") is a support
>      patch needed for context
>    * a6ecfb39ba9d ("usb: hso: fix error handling code of hso_create_net_device")
>      is the actual fix
>    * dcb713d53e2e ("usb: hso: remove the bailout parameter") is a follow up
>      cleanup commit
> 
> Dongliang Mu (2):
>   usb: hso: fix error handling code of hso_create_net_device
>   usb: hso: remove the bailout parameter
> 
> Oliver Neukum (1):
>   hso: fix bailout in error case of probe
> 
>  drivers/net/usb/hso.c | 33 +++++++++++++++++++++++----------
>  1 file changed, 23 insertions(+), 10 deletions(-)

Noticing you sent this patch series for 4.14, 4.19 and 5.4 but am I
right that the last commit dcb713d53e2e ("usb: hso: remove the bailout
parameter") as cleanup commit should ideally as well be applied to
5.10.y and 5.14.y?

Whilst it's probably not strictly needed it would otherwise leave the
upper 5.10.y and 5.14.y inconsistent with those where these series are
applied.

Regards,
Salvatore

Re: [PATCH 5.4 0/3] usb: hso: backport CVE-2021-37159 fix

[Ovidiu Panait]

Hi Salvatore,

On 9/28/21 10:29 PM, Salvatore Bonaccorso wrote:
> [Please note: This e-mail is from an EXTERNAL e-mail address]
>
> Hi Ovidiu
>
> On Tue, Sep 28, 2021 at 04:15:20PM +0300, Ovidiu Panait wrote:
>> All 3 upstream commits apply cleanly:
>>     * 5fcfb6d0bfcd ("hso: fix bailout in error case of probe") is a support
>>       patch needed for context
>>     * a6ecfb39ba9d ("usb: hso: fix error handling code of hso_create_net_device")
>>       is the actual fix
>>     * dcb713d53e2e ("usb: hso: remove the bailout parameter") is a follow up
>>       cleanup commit
>>
>> Dongliang Mu (2):
>>    usb: hso: fix error handling code of hso_create_net_device
>>    usb: hso: remove the bailout parameter
>>
>> Oliver Neukum (1):
>>    hso: fix bailout in error case of probe
>>
>>   drivers/net/usb/hso.c | 33 +++++++++++++++++++++++----------
>>   1 file changed, 23 insertions(+), 10 deletions(-)
> Noticing you sent this patch series for 4.14, 4.19 and 5.4 but am I
> right that the last commit dcb713d53e2e ("usb: hso: remove the bailout
> parameter") as cleanup commit should ideally as well be applied to
> 5.10.y and 5.14.y?
>
> Whilst it's probably not strictly needed it would otherwise leave the
> upper 5.10.y and 5.14.y inconsistent with those where these series are
> applied.

You're right, I have sent the cleanup patch for 5.10/5.14 integration as 
well:

https://lore.kernel.org/stable/20210929075940.1961832-1-ovidiu.panait@windriver.com/T/#t


Thanks!

Ovidiu

> Regards,
> Salvatore

Re: [PATCH 5.4 0/3] usb: hso: backport CVE-2021-37159 fix

[Sasha Levin]

On Wed, Sep 29, 2021 at 11:03:19AM +0300, Ovidiu Panait wrote:
>Hi Salvatore,
>
>On 9/28/21 10:29 PM, Salvatore Bonaccorso wrote:
>>[Please note: This e-mail is from an EXTERNAL e-mail address]
>>
>>Hi Ovidiu
>>
>>On Tue, Sep 28, 2021 at 04:15:20PM +0300, Ovidiu Panait wrote:
>>>All 3 upstream commits apply cleanly:
>>>    * 5fcfb6d0bfcd ("hso: fix bailout in error case of probe") is a support
>>>      patch needed for context
>>>    * a6ecfb39ba9d ("usb: hso: fix error handling code of hso_create_net_device")
>>>      is the actual fix
>>>    * dcb713d53e2e ("usb: hso: remove the bailout parameter") is a follow up
>>>      cleanup commit
>>>
>>>Dongliang Mu (2):
>>>   usb: hso: fix error handling code of hso_create_net_device
>>>   usb: hso: remove the bailout parameter
>>>
>>>Oliver Neukum (1):
>>>   hso: fix bailout in error case of probe
>>>
>>>  drivers/net/usb/hso.c | 33 +++++++++++++++++++++++----------
>>>  1 file changed, 23 insertions(+), 10 deletions(-)
>>Noticing you sent this patch series for 4.14, 4.19 and 5.4 but am I
>>right that the last commit dcb713d53e2e ("usb: hso: remove the bailout
>>parameter") as cleanup commit should ideally as well be applied to
>>5.10.y and 5.14.y?
>>
>>Whilst it's probably not strictly needed it would otherwise leave the
>>upper 5.10.y and 5.14.y inconsistent with those where these series are
>>applied.
>
>You're right, I have sent the cleanup patch for 5.10/5.14 integration 
>as well:
>
>https://lore.kernel.org/stable/20210929075940.1961832-1-ovidiu.panait@windriver.com/T/#t

Why do we need that cleanup commit in <=5.4 to begin with? Does it
actually fix anything?

-- 
Thanks,
Sasha

Re: [PATCH 5.4 0/3] usb: hso: backport CVE-2021-37159 fix

[Ovidiu Panait]

Hi Sasha,

On 10/1/21 7:55 PM, Sasha Levin wrote:
> [Please note: This e-mail is from an EXTERNAL e-mail address]
>
> On Wed, Sep 29, 2021 at 11:03:19AM +0300, Ovidiu Panait wrote:
>> Hi Salvatore,
>>
>> On 9/28/21 10:29 PM, Salvatore Bonaccorso wrote:
>>> [Please note: This e-mail is from an EXTERNAL e-mail address]
>>>
>>> Hi Ovidiu
>>>
>>> On Tue, Sep 28, 2021 at 04:15:20PM +0300, Ovidiu Panait wrote:
>>>> All 3 upstream commits apply cleanly:
>>>>    * 5fcfb6d0bfcd ("hso: fix bailout in error case of probe") is a 
>>>> support
>>>>      patch needed for context
>>>>    * a6ecfb39ba9d ("usb: hso: fix error handling code of 
>>>> hso_create_net_device")
>>>>      is the actual fix
>>>>    * dcb713d53e2e ("usb: hso: remove the bailout parameter") is a 
>>>> follow up
>>>>      cleanup commit
>>>>
>>>> Dongliang Mu (2):
>>>>   usb: hso: fix error handling code of hso_create_net_device
>>>>   usb: hso: remove the bailout parameter
>>>>
>>>> Oliver Neukum (1):
>>>>   hso: fix bailout in error case of probe
>>>>
>>>>  drivers/net/usb/hso.c | 33 +++++++++++++++++++++++----------
>>>>  1 file changed, 23 insertions(+), 10 deletions(-)
>>> Noticing you sent this patch series for 4.14, 4.19 and 5.4 but am I
>>> right that the last commit dcb713d53e2e ("usb: hso: remove the bailout
>>> parameter") as cleanup commit should ideally as well be applied to
>>> 5.10.y and 5.14.y?
>>>
>>> Whilst it's probably not strictly needed it would otherwise leave the
>>> upper 5.10.y and 5.14.y inconsistent with those where these series are
>>> applied.
>>
>> You're right, I have sent the cleanup patch for 5.10/5.14 integration
>> as well:
>>
>> https://lore.kernel.org/stable/20210929075940.1961832-1-ovidiu.panait@windriver.com/T/#t 
>>
>
> Why do we need that cleanup commit in <=5.4 to begin with? Does it
> actually fix anything?
>
The cleanup patch was part of the same patchset with a6ecfb39ba9d ("usb: 
hso: fix error handling code of hso_create_net_device") fix .


I think it can be dropped, as it doesn't seem to fix anything. Can only 
the first two commits be cherry-picked for <=5.4, or should I resend?


Ovidiu

> -- 
> Thanks,
> Sasha

Re: [PATCH 5.4 0/3] usb: hso: backport CVE-2021-37159 fix

[Salvatore Bonaccorso]

Hi,

On Sat, Oct 02, 2021 at 03:36:21PM +0300, Ovidiu Panait wrote:
> Hi Sasha,
> 
> On 10/1/21 7:55 PM, Sasha Levin wrote:
> > [Please note: This e-mail is from an EXTERNAL e-mail address]
> > 
> > On Wed, Sep 29, 2021 at 11:03:19AM +0300, Ovidiu Panait wrote:
> > > Hi Salvatore,
> > > 
> > > On 9/28/21 10:29 PM, Salvatore Bonaccorso wrote:
> > > > [Please note: This e-mail is from an EXTERNAL e-mail address]
> > > > 
> > > > Hi Ovidiu
> > > > 
> > > > On Tue, Sep 28, 2021 at 04:15:20PM +0300, Ovidiu Panait wrote:
> > > > > All 3 upstream commits apply cleanly:
> > > > >    * 5fcfb6d0bfcd ("hso: fix bailout in error case of
> > > > > probe") is a support
> > > > >      patch needed for context
> > > > >    * a6ecfb39ba9d ("usb: hso: fix error handling code of
> > > > > hso_create_net_device")
> > > > >      is the actual fix
> > > > >    * dcb713d53e2e ("usb: hso: remove the bailout parameter")
> > > > > is a follow up
> > > > >      cleanup commit
> > > > > 
> > > > > Dongliang Mu (2):
> > > > >   usb: hso: fix error handling code of hso_create_net_device
> > > > >   usb: hso: remove the bailout parameter
> > > > > 
> > > > > Oliver Neukum (1):
> > > > >   hso: fix bailout in error case of probe
> > > > > 
> > > > >  drivers/net/usb/hso.c | 33 +++++++++++++++++++++++----------
> > > > >  1 file changed, 23 insertions(+), 10 deletions(-)
> > > > Noticing you sent this patch series for 4.14, 4.19 and 5.4 but am I
> > > > right that the last commit dcb713d53e2e ("usb: hso: remove the bailout
> > > > parameter") as cleanup commit should ideally as well be applied to
> > > > 5.10.y and 5.14.y?
> > > > 
> > > > Whilst it's probably not strictly needed it would otherwise leave the
> > > > upper 5.10.y and 5.14.y inconsistent with those where these series are
> > > > applied.
> > > 
> > > You're right, I have sent the cleanup patch for 5.10/5.14 integration
> > > as well:
> > > 
> > > https://lore.kernel.org/stable/20210929075940.1961832-1-ovidiu.panait@windriver.com/T/#t
> > > 
> > 
> > Why do we need that cleanup commit in <=5.4 to begin with? Does it
> > actually fix anything?
> > 
> The cleanup patch was part of the same patchset with a6ecfb39ba9d ("usb:
> hso: fix error handling code of hso_create_net_device") fix .
> 
> 
> I think it can be dropped, as it doesn't seem to fix anything. Can only the
> first two commits be cherry-picked for <=5.4, or should I resend?

Probably the right thing to do, Sasha and Ovidiu. Picking it would
have the small advantage of future commits to backport which would
conflict around the changed lines.

But I have no voice on that matter, I was really only going thorugh
some stable commits backports request covering CVEs and noticed the
submission and it's discrepancy.

For Debian I have for now picked all three commits on top of 4.19.208.

Regards,
Salvatore

Re: [PATCH 5.4 0/3] usb: hso: backport CVE-2021-37159 fix

[Greg KH]

On Tue, Sep 28, 2021 at 04:15:20PM +0300, Ovidiu Panait wrote:
> All 3 upstream commits apply cleanly:
>    * 5fcfb6d0bfcd ("hso: fix bailout in error case of probe") is a support
>      patch needed for context
>    * a6ecfb39ba9d ("usb: hso: fix error handling code of hso_create_net_device")
>      is the actual fix
>    * dcb713d53e2e ("usb: hso: remove the bailout parameter") is a follow up
>      cleanup commit
> 
> Dongliang Mu (2):
>   usb: hso: fix error handling code of hso_create_net_device
>   usb: hso: remove the bailout parameter
> 
> Oliver Neukum (1):
>   hso: fix bailout in error case of probe
> 
>  drivers/net/usb/hso.c | 33 +++++++++++++++++++++++----------
>  1 file changed, 23 insertions(+), 10 deletions(-)
> 
> -- 
> 2.25.1

All now queued up, thanks.

greg k-h