Date:   Tue, 21 Dec 2021 08:42:37 +0100
From:   Greg KH <gregkh@linuxfoundation.org>
To:     Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
Cc:     Larry.Finger@lwfinger.net, phil@philpotter.co.uk,
        linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org,
        Abaci Robot <abaci@linux.alibaba.com>
Subject: Re: [PATCH v3] staging: r8188eu: Use strndup_user instead of
 kmalloc/copy_from_user
Message-ID: <YcGFbUXOAVoxjDB0@kroah.com>
References: <20211213092558.77624-1-jiapeng.chong@linux.alibaba.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211213092558.77624-1-jiapeng.chong@linux.alibaba.com>
Precedence: bulk

On Mon, Dec 13, 2021 at 05:25:58PM +0800, Jiapeng Chong wrote:
> This code assumes that the user is going to give us a NUL terminated
> string which is not necessarily true.
> 
> Fix following coccicheck warning:
> 
> ./drivers/staging/r8188eu/os_dep/ioctl_linux.c:4253:8-15: WARNING
> opportunity for memdup_user.
> 
> Reported-by: Abaci Robot <abaci@linux.alibaba.com>
> Fixes: 2b42bd58b321 ("staging: r8188eu: introduce new os_dep dir for RTL8188eu driver")
> Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
> ---
> Changes in v3:
>   -Delete useless printk, modified the commit message and fixes tag.
> 
>  drivers/staging/r8188eu/os_dep/ioctl_linux.c | 14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/os_dep/ioctl_linux.c b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> index 77728ba78d76..d8c28f279aa0 100644
> --- a/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> +++ b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> @@ -4247,18 +4247,10 @@ static int rtw_test(
>  
>  	DBG_88E("+%s\n", __func__);
>  	len = wrqu->data.length;
> +	pbuf = strndup_user(wrqu->data.pointer, len);
> +	if (IS_ERR(pbuf))
> +		return PTR_ERR(pbuf);
>  
> -	pbuf = kzalloc(len, GFP_KERNEL);
> -	if (!pbuf) {
> -		DBG_88E("%s: no memory!\n", __func__);
> -		return -ENOMEM;
> -	}
> -
> -	if (copy_from_user(pbuf, wrqu->data.pointer, len)) {
> -		kfree(pbuf);
> -		DBG_88E("%s: copy from user fail!\n", __func__);
> -		return -EFAULT;
> -	}
>  	DBG_88E("%s: string =\"%s\"\n", __func__, pbuf);
>  
>  	ptmp = (char *)pbuf;
> -- 
> 2.20.1.7.g153144c
> 
> 

This function does nothing, so it should just be deleted entirely.
Please let's not have odd custom ioctls hanging around that can do
nothing but cause problems.

Also, this function looks like a lovely way to crash a kernel, even with
your fix, so the sooner we get it removed the better.

thanks,

greg k-h