Thadeu Lima de Souza Cascardo <cascardo@xxxxxxxxxx> wrote:
> 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.
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.
