| Message ID | 20241107133322.855112-4-cascardo@igalia.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Ping-Ke Shih |
| Headers | show |
| Series | wifi: rtlwifi: usb probe error path fixes | expand |
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
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 >
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.
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:
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(-)