[PATCH net 0/5] wifi: rtlwifi: usb probe error path fixes

[PATCH net 0/5] wifi: rtlwifi: usb probe error path fixes

[Thadeu Lima de Souza Cascardo]

These are fixes that affect mostly the usb probe error path. It fixes UAF
due to firmware loading touching freed memory by waiting for the load
completion before releasing that memory. It also fixes a couple of
identified memory leaks.

Thadeu Lima de Souza Cascardo (5):
  wifi: rtlwifi: do not complete firmware loading needlessly
  wifi: rtlwifi: rtl8192se: rise completion of firmware loading as last
    step
  wifi: rtlwifi: wait for firmware loading before releasing memory
  wifi: rtlwifi: fix init_sw_vars leak when probe fails
  wifi: rtlwifi: usb: fix workqueue leak when probe fails

 drivers/net/wireless/realtek/rtlwifi/pci.c          | 1 -
 drivers/net/wireless/realtek/rtlwifi/rtl8192se/sw.c | 7 ++++---
 drivers/net/wireless/realtek/rtlwifi/usb.c          | 7 +++++--
 3 files changed, 9 insertions(+), 6 deletions(-)

-- 
2.34.1

[PATCH net 1/5] wifi: rtlwifi: do not complete firmware loading needlessly

[Thadeu Lima de Souza Cascardo]

The only code waiting for completion is driver removal, which will not be
called when probe returns a failure. So this completion is unnecessary.

Fixes: b0302aba812b ("rtlwifi: Convert to asynchronous firmware load")
Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
---
 drivers/net/wireless/realtek/rtlwifi/pci.c | 1 -
 drivers/net/wireless/realtek/rtlwifi/usb.c | 1 -
 2 files changed, 2 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/pci.c b/drivers/net/wireless/realtek/rtlwifi/pci.c
index 11709b6c83f1..40fc3c297a8a 100644
--- a/drivers/net/wireless/realtek/rtlwifi/pci.c
+++ b/drivers/net/wireless/realtek/rtlwifi/pci.c
@@ -2266,7 +2266,6 @@ int rtl_pci_probe(struct pci_dev *pdev,
 		pci_iounmap(pdev, (void __iomem *)rtlpriv->io.pci_mem_start);
 
 	pci_release_regions(pdev);
-	complete(&rtlpriv->firmware_loading_complete);
 
 fail1:
 	if (hw)
diff --git a/drivers/net/wireless/realtek/rtlwifi/usb.c b/drivers/net/wireless/realtek/rtlwifi/usb.c
index d37a017b2b81..c3aa0cd9ff21 100644
--- a/drivers/net/wireless/realtek/rtlwifi/usb.c
+++ b/drivers/net/wireless/realtek/rtlwifi/usb.c
@@ -1040,7 +1040,6 @@ int rtl_usb_probe(struct usb_interface *intf,
 error_out2:
 	_rtl_usb_io_handler_release(hw);
 	usb_put_dev(udev);
-	complete(&rtlpriv->firmware_loading_complete);
 	kfree(rtlpriv->usb_data);
 	ieee80211_free_hw(hw);
 	return -ENODEV;
-- 
2.34.1

RE: [PATCH net 1/5] wifi: rtlwifi: do not complete firmware loading needlessly

[Ping-Ke Shih]

Thadeu Lima de Souza Cascardo <cascardo@igalia.com> wrote:
> The only code waiting for completion is driver removal, which will not be
> called when probe returns a failure. So this completion is unnecessary.
> 
> Fixes: b0302aba812b ("rtlwifi: Convert to asynchronous firmware load")
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>

Acked-by: Ping-Ke Shih <pkshih@realtek.com>

Re: [PATCH net 1/5] wifi: rtlwifi: do not complete firmware loading needlessly

[Ping-Ke Shih]

Thadeu Lima de Souza Cascardo <cascardo@igalia.com> wrote:

> The only code waiting for completion is driver removal, which will not be
> called when probe returns a failure. So this completion is unnecessary.
> 
> Fixes: b0302aba812b ("rtlwifi: Convert to asynchronous firmware load")
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
> Acked-by: Ping-Ke Shih <pkshih@realtek.com>

5 patch(es) applied to rtw-next branch of rtw.git, thanks.

e73e11d30394 wifi: rtlwifi: do not complete firmware loading needlessly
8559a9e0c457 wifi: rtlwifi: rtl8192se: rise completion of firmware loading as last step
b4b26642b31e wifi: rtlwifi: wait for firmware loading before releasing memory
00260350aed8 wifi: rtlwifi: fix init_sw_vars leak when probe fails
f79bc5c67867 wifi: rtlwifi: usb: fix workqueue leak when probe fails

---
https://github.com/pkshih/rtw.git

[PATCH net 2/5] wifi: rtlwifi: rtl8192se: rise completion of firmware loading as last step

[Thadeu Lima de Souza Cascardo]

Just like in commit 4dfde294b979 ("rtlwifi: rise completion at the last
step of firmware callback"), only signal completion once the function is
finished. Otherwise, the module removal waiting for the completion could
free the memory that the callback will still use before returning.

Fixes: b0302aba812b ("rtlwifi: Convert to asynchronous firmware load")
Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
---
 drivers/net/wireless/realtek/rtlwifi/rtl8192se/sw.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192se/sw.c b/drivers/net/wireless/realtek/rtlwifi/rtl8192se/sw.c
index bbf8ff63dced..e63c67b1861b 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8192se/sw.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192se/sw.c
@@ -64,22 +64,23 @@ static void rtl92se_fw_cb(const struct firmware *firmware, void *context)
 
 	rtl_dbg(rtlpriv, COMP_ERR, DBG_LOUD,
 		"Firmware callback routine entered!\n");
-	complete(&rtlpriv->firmware_loading_complete);
 	if (!firmware) {
 		pr_err("Firmware %s not available\n", fw_name);
 		rtlpriv->max_fw_size = 0;
-		return;
+		goto exit;
 	}
 	if (firmware->size > rtlpriv->max_fw_size) {
 		pr_err("Firmware is too big!\n");
 		rtlpriv->max_fw_size = 0;
 		release_firmware(firmware);
-		return;
+		goto exit;
 	}
 	pfirmware = (struct rt_firmware *)rtlpriv->rtlhal.pfirmware;
 	memcpy(pfirmware->sz_fw_tmpbuffer, firmware->data, firmware->size);
 	pfirmware->sz_fw_tmpbufferlen = firmware->size;
 	release_firmware(firmware);
+exit:
+	complete(&rtlpriv->firmware_loading_complete);
 }
 
 static int rtl92s_init_sw_vars(struct ieee80211_hw *hw)
-- 
2.34.1

RE: [PATCH net 2/5] wifi: rtlwifi: rtl8192se: rise completion of firmware loading as last step

[Ping-Ke Shih]

Thadeu Lima de Souza Cascardo <cascardo@igalia.com> wrote:
> Just like in commit 4dfde294b979 ("rtlwifi: rise completion at the last
> step of firmware callback"), only signal completion once the function is
> finished. Otherwise, the module removal waiting for the completion could
> free the memory that the callback will still use before returning.
> 
> Fixes: b0302aba812b ("rtlwifi: Convert to asynchronous firmware load")
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>

Acked-by: Ping-Ke Shih <pkshih@realtek.com>

[PATCH net 3/5] wifi: rtlwifi: wait for firmware loading before releasing memory

[Thadeu Lima de Souza Cascardo]

At probe error path, the firmware loading work may have already been
queued. In such a case, it will try to access memory allocated by the probe
function, which is about to be released. In such paths, wait for the
firmware worker to finish before releasing memory.

Fixes: a7f7c15e945a ("rtlwifi: rtl8192cu: Free ieee80211_hw if probing fails")
Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
---
 drivers/net/wireless/realtek/rtlwifi/usb.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/usb.c b/drivers/net/wireless/realtek/rtlwifi/usb.c
index c3aa0cd9ff21..c27b116ccdff 100644
--- a/drivers/net/wireless/realtek/rtlwifi/usb.c
+++ b/drivers/net/wireless/realtek/rtlwifi/usb.c
@@ -1028,13 +1028,15 @@ int rtl_usb_probe(struct usb_interface *intf,
 	err = ieee80211_register_hw(hw);
 	if (err) {
 		pr_err("Can't register mac80211 hw.\n");
-		goto error_out;
+		goto error_init_vars;
 	}
 	rtlpriv->mac80211.mac80211_registered = 1;
 
 	set_bit(RTL_STATUS_INTERFACE_START, &rtlpriv->status);
 	return 0;
 
+error_init_vars:
+	wait_for_completion(&rtlpriv->firmware_loading_complete);
 error_out:
 	rtl_deinit_core(hw);
 error_out2:
-- 
2.34.1

RE: [PATCH net 3/5] wifi: rtlwifi: wait for firmware loading before releasing memory

[Ping-Ke Shih]

Thadeu Lima de Souza Cascardo <cascardo@igalia.com> wrote:
> At probe error path, the firmware loading work may have already been
> queued. In such a case, it will try to access memory allocated by the probe
> function, which is about to be released. In such paths, wait for the
> firmware worker to finish before releasing memory.
> 
> Fixes: a7f7c15e945a ("rtlwifi: rtl8192cu: Free ieee80211_hw if probing fails")
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
> ---
>  drivers/net/wireless/realtek/rtlwifi/usb.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtlwifi/usb.c b/drivers/net/wireless/realtek/rtlwifi/usb.c
> index c3aa0cd9ff21..c27b116ccdff 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/usb.c
> +++ b/drivers/net/wireless/realtek/rtlwifi/usb.c
> @@ -1028,13 +1028,15 @@ int rtl_usb_probe(struct usb_interface *intf,
>         err = ieee80211_register_hw(hw);
>         if (err) {
>                 pr_err("Can't register mac80211 hw.\n");
> -               goto error_out;
> +               goto error_init_vars;
>         }
>         rtlpriv->mac80211.mac80211_registered = 1;
> 
>         set_bit(RTL_STATUS_INTERFACE_START, &rtlpriv->status);
>         return 0;
> 
> +error_init_vars:
> +       wait_for_completion(&rtlpriv->firmware_loading_complete);

The firmware request is trigged by rtlpriv->cfg->ops->init_sw_vars(hw), and
here is wait for filling rtlpriv->rtlhal.pfirmware and
rtlpriv->rtlhal.wowlan_firmware.
 
The rtlpriv->cfg->ops->deinit_sw_vars(hw) is to free firmware. Shouldn't we
call it here? Also shouldn't PCI need this?

>  error_out:
>         rtl_deinit_core(hw);
>  error_out2:
> --
> 2.34.1

Re: [PATCH net 3/5] wifi: rtlwifi: wait for firmware loading before releasing memory

[Thadeu Lima de Souza Cascardo]

On Fri, Nov 08, 2024 at 02:12:24AM +0000, Ping-Ke Shih wrote:
> Thadeu Lima de Souza Cascardo <cascardo@igalia.com> wrote:
> > At probe error path, the firmware loading work may have already been
> > queued. In such a case, it will try to access memory allocated by the probe
> > function, which is about to be released. In such paths, wait for the
> > firmware worker to finish before releasing memory.
> > 
> > Fixes: a7f7c15e945a ("rtlwifi: rtl8192cu: Free ieee80211_hw if probing fails")
> > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
> > ---
> >  drivers/net/wireless/realtek/rtlwifi/usb.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/wireless/realtek/rtlwifi/usb.c b/drivers/net/wireless/realtek/rtlwifi/usb.c
> > index c3aa0cd9ff21..c27b116ccdff 100644
> > --- a/drivers/net/wireless/realtek/rtlwifi/usb.c
> > +++ b/drivers/net/wireless/realtek/rtlwifi/usb.c
> > @@ -1028,13 +1028,15 @@ int rtl_usb_probe(struct usb_interface *intf,
> >         err = ieee80211_register_hw(hw);
> >         if (err) {
> >                 pr_err("Can't register mac80211 hw.\n");
> > -               goto error_out;
> > +               goto error_init_vars;
> >         }
> >         rtlpriv->mac80211.mac80211_registered = 1;
> > 
> >         set_bit(RTL_STATUS_INTERFACE_START, &rtlpriv->status);
> >         return 0;
> > 
> > +error_init_vars:
> > +       wait_for_completion(&rtlpriv->firmware_loading_complete);
> 
> The firmware request is trigged by rtlpriv->cfg->ops->init_sw_vars(hw), and
> here is wait for filling rtlpriv->rtlhal.pfirmware and
> rtlpriv->rtlhal.wowlan_firmware.
>  
> The rtlpriv->cfg->ops->deinit_sw_vars(hw) is to free firmware. Shouldn't we
> call it here? Also shouldn't PCI need this?
> 

About PCI, as I was testing with a USB emulator, I couldn't test it, though
I found a few fixes that it would need as well. I can send a patchset for
the PCI fixes, though I won't be able to test them.

As for merging the following patch with this one, it would make sense. But
I found out the memory leak after coming up with this fix. And instead of
explaining two bugs (one UAF and one memory leak) in the same commit, I
thought it more simple to have two commits. Besides, given the Fixes line I
came up with for each one of them, they would apply to different trees.

Cascardo.

> >  error_out:
> >         rtl_deinit_core(hw);
> >  error_out2:
> > --
> > 2.34.1
> 

RE: [PATCH net 3/5] wifi: rtlwifi: wait for firmware loading before releasing memory

[Ping-Ke Shih]

Thadeu Lima de Souza Cascardo <cascardo@igalia.com> wrote:
> On Fri, Nov 08, 2024 at 02:12:24AM +0000, Ping-Ke Shih wrote:
> > Thadeu Lima de Souza Cascardo <cascardo@igalia.com> wrote:
> > > At probe error path, the firmware loading work may have already been
> > > queued. In such a case, it will try to access memory allocated by the probe
> > > function, which is about to be released. In such paths, wait for the
> > > firmware worker to finish before releasing memory.
> > >
> > > Fixes: a7f7c15e945a ("rtlwifi: rtl8192cu: Free ieee80211_hw if probing fails")
> > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
> > > ---
> > >  drivers/net/wireless/realtek/rtlwifi/usb.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/wireless/realtek/rtlwifi/usb.c b/drivers/net/wireless/realtek/rtlwifi/usb.c
> > > index c3aa0cd9ff21..c27b116ccdff 100644
> > > --- a/drivers/net/wireless/realtek/rtlwifi/usb.c
> > > +++ b/drivers/net/wireless/realtek/rtlwifi/usb.c
> > > @@ -1028,13 +1028,15 @@ int rtl_usb_probe(struct usb_interface *intf,
> > >         err = ieee80211_register_hw(hw);
> > >         if (err) {
> > >                 pr_err("Can't register mac80211 hw.\n");
> > > -               goto error_out;
> > > +               goto error_init_vars;
> > >         }
> > >         rtlpriv->mac80211.mac80211_registered = 1;
> > >
> > >         set_bit(RTL_STATUS_INTERFACE_START, &rtlpriv->status);
> > >         return 0;
> > >
> > > +error_init_vars:
> > > +       wait_for_completion(&rtlpriv->firmware_loading_complete);
> >
> > The firmware request is trigged by rtlpriv->cfg->ops->init_sw_vars(hw), and
> > here is wait for filling rtlpriv->rtlhal.pfirmware and
> > rtlpriv->rtlhal.wowlan_firmware.
> >
> > The rtlpriv->cfg->ops->deinit_sw_vars(hw) is to free firmware. Shouldn't we
> > call it here? Also shouldn't PCI need this?
> >
> 
> About PCI, as I was testing with a USB emulator, I couldn't test it, though
> I found a few fixes that it would need as well. I can send a patchset for
> the PCI fixes, though I won't be able to test them.

Thanks for the PCI fixes in advance. 

> 
> As for merging the following patch with this one, it would make sense. But
> I found out the memory leak after coming up with this fix. And instead of
> explaining two bugs (one UAF and one memory leak) in the same commit, I
> thought it more simple to have two commits. Besides, given the Fixes line I
> came up with for each one of them, they would apply to different trees.

I feel these two commits are dependent. It is hard to pick latter one without
taking this first. But I think it is fine to keep it as was. 

[PATCH net 4/5] wifi: rtlwifi: fix init_sw_vars leak when probe fails

[Thadeu Lima de Souza Cascardo]

If ieee80211_register_hw fails, the memory allocated for the firmware will
not be released. Call deinit_sw_vars as the function that undoes the
allocationes done by init_sw_vars.

Fixes: cefe3dfdb9f5 ("rtl8192cu: Call ieee80211_register_hw from rtl_usb_probe")
Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
---
 drivers/net/wireless/realtek/rtlwifi/usb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/wireless/realtek/rtlwifi/usb.c b/drivers/net/wireless/realtek/rtlwifi/usb.c
index c27b116ccdff..8ec687fab572 100644
--- a/drivers/net/wireless/realtek/rtlwifi/usb.c
+++ b/drivers/net/wireless/realtek/rtlwifi/usb.c
@@ -1037,6 +1037,7 @@ int rtl_usb_probe(struct usb_interface *intf,
 
 error_init_vars:
 	wait_for_completion(&rtlpriv->firmware_loading_complete);
+	rtlpriv->cfg->ops->deinit_sw_vars(hw);
 error_out:
 	rtl_deinit_core(hw);
 error_out2:
-- 
2.34.1

RE: [PATCH net 4/5] wifi: rtlwifi: fix init_sw_vars leak when probe fails

[Ping-Ke Shih]

Thadeu Lima de Souza Cascardo <cascardo@igalia.com> wrote:
> If ieee80211_register_hw fails, the memory allocated for the firmware will
> not be released. Call deinit_sw_vars as the function that undoes the
> allocationes done by init_sw_vars.
> 
> Fixes: cefe3dfdb9f5 ("rtl8192cu: Call ieee80211_register_hw from rtl_usb_probe")
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
> ---
>  drivers/net/wireless/realtek/rtlwifi/usb.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/wireless/realtek/rtlwifi/usb.c b/drivers/net/wireless/realtek/rtlwifi/usb.c
> index c27b116ccdff..8ec687fab572 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/usb.c
> +++ b/drivers/net/wireless/realtek/rtlwifi/usb.c
> @@ -1037,6 +1037,7 @@ int rtl_usb_probe(struct usb_interface *intf,
> 
>  error_init_vars:
>         wait_for_completion(&rtlpriv->firmware_loading_complete);
> +       rtlpriv->cfg->ops->deinit_sw_vars(hw);

Ah. You did this in this patch. Seemingly we can combine these two small
patches. 

>  error_out:
>         rtl_deinit_core(hw);
>  error_out2:
> --
> 2.34.1

[PATCH net 5/5] wifi: rtlwifi: usb: fix workqueue leak when probe fails

[Thadeu Lima de Souza Cascardo]

rtl_init_core creates a workqueue that is then assigned to rtl_wq.
rtl_deinit_core does not destroy it. It is left to rtl_usb_deinit, which
must be called in the probe error path.

Fixes: 2ca20f79e0d8 ("rtlwifi: Add usb driver")
Fixes: 851639fdaeac ("rtlwifi: Modify some USB de-initialize code.")
Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
---
 drivers/net/wireless/realtek/rtlwifi/usb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/wireless/realtek/rtlwifi/usb.c b/drivers/net/wireless/realtek/rtlwifi/usb.c
index 8ec687fab572..0368ecea2e81 100644
--- a/drivers/net/wireless/realtek/rtlwifi/usb.c
+++ b/drivers/net/wireless/realtek/rtlwifi/usb.c
@@ -1039,6 +1039,7 @@ int rtl_usb_probe(struct usb_interface *intf,
 	wait_for_completion(&rtlpriv->firmware_loading_complete);
 	rtlpriv->cfg->ops->deinit_sw_vars(hw);
 error_out:
+	rtl_usb_deinit(hw);
 	rtl_deinit_core(hw);
 error_out2:
 	_rtl_usb_io_handler_release(hw);
-- 
2.34.1

RE: [PATCH net 5/5] wifi: rtlwifi: usb: fix workqueue leak when probe fails

[Ping-Ke Shih]

Thadeu Lima de Souza Cascardo <cascardo@igalia.com> wrote:
> rtl_init_core creates a workqueue that is then assigned to rtl_wq.
> rtl_deinit_core does not destroy it. It is left to rtl_usb_deinit, which
> must be called in the probe error path.
> 
> Fixes: 2ca20f79e0d8 ("rtlwifi: Add usb driver")
> Fixes: 851639fdaeac ("rtlwifi: Modify some USB de-initialize code.")
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
> ---
>  drivers/net/wireless/realtek/rtlwifi/usb.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/wireless/realtek/rtlwifi/usb.c b/drivers/net/wireless/realtek/rtlwifi/usb.c
> index 8ec687fab572..0368ecea2e81 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/usb.c
> +++ b/drivers/net/wireless/realtek/rtlwifi/usb.c
> @@ -1039,6 +1039,7 @@ int rtl_usb_probe(struct usb_interface *intf,
>         wait_for_completion(&rtlpriv->firmware_loading_complete);
>         rtlpriv->cfg->ops->deinit_sw_vars(hw);
>  error_out:
> +       rtl_usb_deinit(hw);
>         rtl_deinit_core(hw);
>  error_out2:
>         _rtl_usb_io_handler_release(hw);

I think deinit should be in reverse order of init step by step:

--- a/drivers/net/wireless/realtek/rtlwifi/usb.c
+++ b/drivers/net/wireless/realtek/rtlwifi/usb.c
@@ -1017,7 +1017,7 @@ int rtl_usb_probe(struct usb_interface *intf,
        err = rtl_init_core(hw);
        if (err) {
                pr_err("Can't allocate sw for mac80211\n");
-               goto error_out2;
+               goto error_out_usb_deinit;
        }
        if (rtlpriv->cfg->ops->init_sw_vars(hw)) {
                pr_err("Can't init_sw_vars\n");
@@ -1040,6 +1040,8 @@ int rtl_usb_probe(struct usb_interface *intf,
        rtlpriv->cfg->ops->deinit_sw_vars(hw);
 error_out:
        rtl_deinit_core(hw);
+error_out_usb_deinit:
+       rtl_usb_deinit(hw);
 error_out2:
        _rtl_usb_io_handler_release(hw);
        usb_put_dev(udev);

Have you also considered PCI? It seems that rtl_pci_deinit() isn't called if
PCI probe fails. 

Re: [PATCH net 5/5] wifi: rtlwifi: usb: fix workqueue leak when probe fails

[Thadeu Lima de Souza Cascardo]

On Fri, Nov 08, 2024 at 02:23:48AM +0000, Ping-Ke Shih wrote:
> Thadeu Lima de Souza Cascardo <cascardo@igalia.com> wrote:
> > rtl_init_core creates a workqueue that is then assigned to rtl_wq.
> > rtl_deinit_core does not destroy it. It is left to rtl_usb_deinit, which
> > must be called in the probe error path.
> > 
> > Fixes: 2ca20f79e0d8 ("rtlwifi: Add usb driver")
> > Fixes: 851639fdaeac ("rtlwifi: Modify some USB de-initialize code.")
> > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
> > ---
> >  drivers/net/wireless/realtek/rtlwifi/usb.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/net/wireless/realtek/rtlwifi/usb.c b/drivers/net/wireless/realtek/rtlwifi/usb.c
> > index 8ec687fab572..0368ecea2e81 100644
> > --- a/drivers/net/wireless/realtek/rtlwifi/usb.c
> > +++ b/drivers/net/wireless/realtek/rtlwifi/usb.c
> > @@ -1039,6 +1039,7 @@ int rtl_usb_probe(struct usb_interface *intf,
> >         wait_for_completion(&rtlpriv->firmware_loading_complete);
> >         rtlpriv->cfg->ops->deinit_sw_vars(hw);
> >  error_out:
> > +       rtl_usb_deinit(hw);
> >         rtl_deinit_core(hw);
> >  error_out2:
> >         _rtl_usb_io_handler_release(hw);
> 
> I think deinit should be in reverse order of init step by step:

Well, I kept the order that they appear in the remove path. Also, I checked
that they are not exactly independent and rtl_usb_deinit does not need to
be called when rtl_init_core fails. I have just checked and it wouldn't
cause any harm either if rtl_usb_deinit is called in case rtl_init_core
fails. So either way should be fine.

Cascardo.

> 
> --- a/drivers/net/wireless/realtek/rtlwifi/usb.c
> +++ b/drivers/net/wireless/realtek/rtlwifi/usb.c
> @@ -1017,7 +1017,7 @@ int rtl_usb_probe(struct usb_interface *intf,
>         err = rtl_init_core(hw);
>         if (err) {
>                 pr_err("Can't allocate sw for mac80211\n");
> -               goto error_out2;
> +               goto error_out_usb_deinit;
>         }
>         if (rtlpriv->cfg->ops->init_sw_vars(hw)) {
>                 pr_err("Can't init_sw_vars\n");
> @@ -1040,6 +1040,8 @@ int rtl_usb_probe(struct usb_interface *intf,
>         rtlpriv->cfg->ops->deinit_sw_vars(hw);
>  error_out:
>         rtl_deinit_core(hw);
> +error_out_usb_deinit:
> +       rtl_usb_deinit(hw);
>  error_out2:
>         _rtl_usb_io_handler_release(hw);
>         usb_put_dev(udev);
> 
> Have you also considered PCI? It seems that rtl_pci_deinit() isn't called if
> PCI probe fails. 
> 
> 

RE: [PATCH net 5/5] wifi: rtlwifi: usb: fix workqueue leak when probe fails

[Ping-Ke Shih]

Thadeu Lima de Souza Cascardo <cascardo@igalia.com> wrote:
> On Fri, Nov 08, 2024 at 02:23:48AM +0000, Ping-Ke Shih wrote:
> > Thadeu Lima de Souza Cascardo <cascardo@igalia.com> wrote:
> > > rtl_init_core creates a workqueue that is then assigned to rtl_wq.
> > > rtl_deinit_core does not destroy it. It is left to rtl_usb_deinit, which
> > > must be called in the probe error path.
> > >
> > > Fixes: 2ca20f79e0d8 ("rtlwifi: Add usb driver")
> > > Fixes: 851639fdaeac ("rtlwifi: Modify some USB de-initialize code.")
> > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
> > > ---
> > >  drivers/net/wireless/realtek/rtlwifi/usb.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/net/wireless/realtek/rtlwifi/usb.c b/drivers/net/wireless/realtek/rtlwifi/usb.c
> > > index 8ec687fab572..0368ecea2e81 100644
> > > --- a/drivers/net/wireless/realtek/rtlwifi/usb.c
> > > +++ b/drivers/net/wireless/realtek/rtlwifi/usb.c
> > > @@ -1039,6 +1039,7 @@ int rtl_usb_probe(struct usb_interface *intf,
> > >         wait_for_completion(&rtlpriv->firmware_loading_complete);
> > >         rtlpriv->cfg->ops->deinit_sw_vars(hw);
> > >  error_out:
> > > +       rtl_usb_deinit(hw);
> > >         rtl_deinit_core(hw);
> > >  error_out2:
> > >         _rtl_usb_io_handler_release(hw);
> >
> > I think deinit should be in reverse order of init step by step:
> 
> Well, I kept the order that they appear in the remove path. Also, I checked
> that they are not exactly independent and rtl_usb_deinit does not need to
> be called when rtl_init_core fails. I have just checked and it wouldn't
> cause any harm either if rtl_usb_deinit is called in case rtl_init_core
> fails. So either way should be fine.

Well. Original implementation of the workqueue is not symmetric. Without
real hardware verification, we can't move the code of destroy queue from
rtl_usb_deinit() to rtl_deinit_core(). 

RE: [PATCH net 0/5] wifi: rtlwifi: usb probe error path fixes

[Ping-Ke Shih]

Thadeu Lima de Souza Cascardo <cascardo@igalia.com> wrote:
> These are fixes that affect mostly the usb probe error path. It fixes UAF
> due to firmware loading touching freed memory by waiting for the load
> completion before releasing that memory. It also fixes a couple of
> identified memory leaks.

This goes via wireless tree, not net. Just send to linux-wireless (you have done).
No need "net" in patch subject. 

I would quickly check if you did really encounter problems and
have tested this patchset with real hardware?

Re: [PATCH net 0/5] wifi: rtlwifi: usb probe error path fixes

[Thadeu Lima de Souza Cascardo]

On Fri, Nov 08, 2024 at 01:41:45AM +0000, Ping-Ke Shih wrote:
> Thadeu Lima de Souza Cascardo <cascardo@igalia.com> wrote:
> > These are fixes that affect mostly the usb probe error path. It fixes UAF
> > due to firmware loading touching freed memory by waiting for the load
> > completion before releasing that memory. It also fixes a couple of
> > identified memory leaks.
> 
> This goes via wireless tree, not net. Just send to linux-wireless (you have done).
> No need "net" in patch subject. 
> 
> I would quickly check if you did really encounter problems and
> have tested this patchset with real hardware?
> 
> 

Yeah, I was playing it safe here, in case some of the same rules apply, and
"PATCH net" was required.

If found this with a reproducer emulating a usb gadget device (by using
/dev/raw-gadget), and then injecting memory allocation failures at
different points in the probe path (at ieee80211_register_hw and then at
init_sw_vars).

I haven't tested this with real hardware, but given this lies in the probe
error path, I suppose it would be harder to test for the bugs that they
fix. On the other hand, it would be nice to at least confirm that it
doesn't break them, though I find it hard that it would.

Thanks.
Cascardo.

Re: [PATCH net 0/5] wifi: rtlwifi: usb probe error path fixes

[Kalle Valo]

Thadeu Lima de Souza Cascardo <cascardo@igalia.com> writes:

> On Fri, Nov 08, 2024 at 01:41:45AM +0000, Ping-Ke Shih wrote:
>> Thadeu Lima de Souza Cascardo <cascardo@igalia.com> wrote:
>> > These are fixes that affect mostly the usb probe error path. It fixes UAF
>> > due to firmware loading touching freed memory by waiting for the load
>> > completion before releasing that memory. It also fixes a couple of
>> > identified memory leaks.
>> 
>> This goes via wireless tree, not net. Just send to linux-wireless (you have done).
>> No need "net" in patch subject. 
>> 
>> I would quickly check if you did really encounter problems and
>> have tested this patchset with real hardware?
>> 
>> 
>
> Yeah, I was playing it safe here, in case some of the same rules apply, and
> "PATCH net" was required.
>
> If found this with a reproducer emulating a usb gadget device (by using
> /dev/raw-gadget), and then injecting memory allocation failures at
> different points in the probe path (at ieee80211_register_hw and then at
> init_sw_vars).
>
> I haven't tested this with real hardware, but given this lies in the probe
> error path, I suppose it would be harder to test for the bugs that they
> fix. On the other hand, it would be nice to at least confirm that it
> doesn't break them, though I find it hard that it would.

Yeah, regressions are what we maintainers are most worried. We certainly
do not want cleanup patches breaking existing setups.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches