linux-kernel - Re: [PATCH] staging: rtl8723bs: os_dep: simplify the return statement.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 11 Oct 2021 15:26:31 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Saurav Girepunje <saurav.girepunje@...il.com>
Cc:     gregkh@...uxfoundation.org, fabioaiuto83@...il.com,
        ross.schm.dev@...il.com, marcocesati@...il.com,
        insafonov@...il.com, linux-staging@...ts.linux.dev,
        linux-kernel@...r.kernel.org, saurav.girepunje@...mail.com
Subject: Re: [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@...il.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