lkml.org 
[lkml]   [2021]   [Oct]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] staging: rtl8723bs: os_dep: simplify the return statement.
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@gmail.com>
> ---
> .../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.

> }
>
> 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.

> +
> +err_zmalloc:
> +
> + free_netdev(mon_ndev);
> + *ndev = mon_ndev = NULL;

mon_ndev is local too.

regards,
dan carpenter


\
 
 \ /
  Last update: 2021-10-11 14:29    [W:0.079 / U:0.380 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site