Date:   Thu, 10 Dec 2020 23:05:34 +0800 (GMT+08:00)
From:   shaojie.dong@...c.iscas.ac.cn
To:     "Dan Carpenter" <dan.carpenter@...cle.com>
Cc:     Larry.Finger@...inger.net, florian.c.schilhabel@...glemail.com,
        gregkh@...uxfoundation.org, devel@...verdev.osuosl.org,
        linux-kernel@...r.kernel.org
Subject: Re: Re: [PATCH] staging: rtl8712: check register_netdev() return
 value

Hi

&gt; 
&gt; This function should not be calling register_netdev().  What does that
&gt; have to do with firmware?  It should also not free_netdev() because
&gt; that will just lead to a use after free in the caller.
&gt;

--&gt; check code history author<larry.finger@...inger.net> changed synchronous firmware loading to asynchronous firmware loading
    before this change, register_netdev() was not calling in firmware related function.
    For asynchronous loading, maybe register_netdev() be calling in rtl871x_load_fw_cb() is to ensure the netdev be registered after firmware loading completed

--&gt; for potential use after free issue
    Could I only call "free_irq(adapter-&gt;pnetdev-&gt;irq, adapter-&gt;pnetdev)" when register_netdev() failed ?
    If no need to change drivers/staging/rtl8712/hal_init.c file, I could give up my patch, thank you !

&gt; -----原始邮件-----
&gt; 发件人: "Dan Carpenter" <dan.carpenter@...cle.com>
&gt; 发送时间: 2020-12-10 01:46:15 (星期四)
&gt; 收件人: shaojie.dong@...c.iscas.ac.cn
&gt; 抄送: Larry.Finger@...inger.net, florian.c.schilhabel@...glemail.com, gregkh@...uxfoundation.org, devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org
&gt; 主题: Re: [PATCH] staging: rtl8712: check register_netdev() return value
&gt; 
&gt; On Wed, Dec 09, 2020 at 11:01:24PM +0800, shaojie.dong@...c.iscas.ac.cn wrote:
&gt; &gt; From: "shaojie.dong" <shaojie.dong@...c.iscas.ac.cn>
&gt; &gt; 
&gt; &gt; Function register_netdev() can fail, so we should check it's return value
&gt; &gt; 
&gt; &gt; Signed-off-by: shaojie.dong <shaojie.dong@...c.iscas.ac.cn>
&gt; &gt; ---
&gt; &gt;  drivers/staging/rtl8712/hal_init.c | 5 ++++-
&gt; &gt;  1 file changed, 4 insertions(+), 1 deletion(-)
&gt; &gt; 
&gt; &gt; diff --git a/drivers/staging/rtl8712/hal_init.c b/drivers/staging/rtl8712/hal_init.c
&gt; &gt; index 715f1fe8b..38a3e3d44 100644
&gt; &gt; --- a/drivers/staging/rtl8712/hal_init.c
&gt; &gt; +++ b/drivers/staging/rtl8712/hal_init.c
&gt; &gt; @@ -45,7 +45,10 @@ static void rtl871x_load_fw_cb(const struct firmware *firmware, void *context)
&gt; &gt;  	}
&gt; &gt;  	adapter-&gt;fw = firmware;
&gt; &gt;  	/* firmware available - start netdev */
&gt; &gt; -	register_netdev(adapter-&gt;pnetdev);
&gt; &gt; +	if (register_netdev(adapter-&gt;pnetdev) != 0) {
&gt; &gt; +		netdev_err(adapter-&gt;pnetdev, "register_netdev() failed\n");
&gt; &gt; +		free_netdev(adapter-&gt;pnetdev);
&gt; &gt; +	}
&gt; 
&gt; This function should not be calling register_netdev().  What does that
&gt; have to do with firmware?  It should also not free_netdev() because
&gt; that will just lead to a use after free in the caller.
&gt; 
&gt; regards,
&gt; dan carpenter
&gt; 
&gt; &gt;  	complete(&amp;adapter-&gt;rtl8712_fw_ready);
&gt; &gt;  }
&gt; &gt;  
&gt; &gt; -- 
&gt; &gt; 2.17.1
&gt; &gt; 
&gt; &gt; _______________________________________________
&gt; &gt; devel mailing list
&gt; &gt; devel@...uxdriverproject.org
&gt; &gt; http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
</shaojie.dong@...c.iscas.ac.cn></shaojie.dong@...c.iscas.ac.cn></dan.carpenter@...cle.com></larry.finger@...inger.net>

Powered by blists - more mailing lists