| Message ID | 20241107133322.855112-1-cascardo@igalia.com |
|---|---|
| Headers | show |
| Series | wifi: rtlwifi: usb probe error path fixes | expand |
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?
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>
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
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.
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.
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 >
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. > >
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.
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().
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.
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(-)