2021-12-13 09:26:16

by Jiapeng Chong

Subject: [PATCH v3] staging: r8188eu: Use strndup_user instead of kmalloc/copy_from_user

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 <[email protected]>
Fixes: 2b42bd58b321 ("staging: r8188eu: introduce new os_dep dir for RTL8188eu driver")
Signed-off-by: Jiapeng Chong <[email protected]>
---
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

2021-12-21 07:42:43

by Greg Kroah-Hartman

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 <[email protected]>
> Fixes: 2b42bd58b321 ("staging: r8188eu: introduce new os_dep dir for RTL8188eu driver")
> Signed-off-by: Jiapeng Chong <[email protected]>
> ---
> 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