linux-kernel - Re: [PATCH v3] staging: r8188eu: Use strndup_user instead of kmalloc/copy_from

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] [day] [month] [year] [list]

Message-ID: <YcGFbUXOAVoxjDB0@kroah.com>
Date:   Tue, 21 Dec 2021 08:42:37 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Jiapeng Chong <jiapeng.chong@...ux.alibaba.com>
Cc:     Larry.Finger@...inger.net, phil@...lpotter.co.uk,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org,
        Abaci Robot <abaci@...ux.alibaba.com>
Subject: Re: [PATCH v3] staging: r8188eu: Use strndup_user instead of
 kmalloc/copy_from_user

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@...ux.alibaba.com>
> Fixes: 2b42bd58b321 ("staging: r8188eu: introduce new os_dep dir for RTL8188eu driver")
> Signed-off-by: Jiapeng Chong <jiapeng.chong@...ux.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