On Fri, Nov 08, 2024 at 02:12:24AM +0000, Ping-Ke Shih wrote:
> Thadeu Lima de Souza Cascardo <cascardo@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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
>
