On 11/10/21 5:56 pm, Dan Carpenter wrote:
> This introduces a use after free on the sucess path. You need to be a
> lot more careful.
>
> On Sat, Oct 09, 2021 at 09:09:12PM +0530, Saurav Girepunje wrote:
>> Remove the unneeded and redundant check of variable on goto out.
>> Simplify the return using multiple goto label to avoid
>> unneeded check.
>>
>> Signed-off-by: Saurav Girepunje <saurav.girepunje@xxxxxxxxx>
>> ---
>> .../staging/rtl8723bs/os_dep/ioctl_cfg80211.c | 29 ++++++++++---------
>> 1 file changed, 15 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
>> index 0868f56e2979..574fdb6adce7 100644
>> --- a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
>> +++ b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
>> @@ -2282,18 +2282,18 @@ static int rtw_cfg80211_add_monitor_if(struct adapter *padapter, char *name, str
>>
>> if (!name) {
>> ret = -EINVAL;
>> - goto out;
>> + goto err_out;
>
> Just return directly. "return -EINVAL;" but what does "goto err_out;"
> do? No one knows without scrolling down to the very bottom of the
> function, then scrolling all the way up again. At this point you have
> lost your place in the code and your train of thought is de-railed.
>
> Plus it introduces "forgot to set the error code" bugs.
>
>> @@ -2312,7 +2312,7 @@ static int rtw_cfg80211_add_monitor_if(struct adapter *padapter, char *name, str
>> mon_wdev = rtw_zmalloc(sizeof(struct wireless_dev));
>> if (!mon_wdev) {
>> ret = -ENOMEM;
>> - goto out;
>> + goto err_zmalloc;
>
>
> This is a Come From style naming. Imagine if instead of naming functions
> after what they do we instead named them after the first caller which
> was introduced. kmalloc() would be named called_from_boot_510(). It's
> a usless naming scheme. We have to scroll down to the bottom to see
> what it does.
>
I will send another patch to remove goto and simply return ret value.
>> }
>>
>> mon_wdev->wiphy = padapter->rtw_wdev->wiphy;
>> @@ -2322,22 +2322,23 @@ static int rtw_cfg80211_add_monitor_if(struct adapter *padapter, char *name, str
>>
>> ret = cfg80211_register_netdevice(mon_ndev);
>> if (ret) {
>> - goto out;
>> + goto err_register;
>> }
>>
>> *ndev = pwdev_priv->pmon_ndev = mon_ndev;
>> memcpy(pwdev_priv->ifname_mon, name, IFNAMSIZ+1);
>>
>> -out:
>> - if (ret && mon_wdev) {
>> - kfree(mon_wdev);
>> - mon_wdev = NULL;
>> - }
>> +err_register:
>>
>> - if (ret && mon_ndev) {
>> - free_netdev(mon_ndev);
>> - *ndev = mon_ndev = NULL;
>> - }
>> + kfree(mon_wdev);
>> + mon_wdev = NULL;
>
> This is an on stack variable. Think about what you are doing. You're
> not writing carefully at all.
>
I didn't removed local variable assignment to NULL on this patch.
However I agree this is another improvement possible on this function and can be done along with other changes.
Please let me know you opinion whether I should send one patch or multiple patch in a single series.
>> +
>> +err_zmalloc:
>> +
>> + free_netdev(mon_ndev);
>> + *ndev = mon_ndev = NULL;
>
> mon_ndev is local too.
>
> regards,
> dan carpenter
>
>
Regards,
Saurav
