diff mbox series

[v2,1/2] wifi: rtw88: fix incorrect error codes in rtw_debugfs_copy_from_user

Message ID tencent_8BA35E4B9CDF40D4AE6D8D831D7ACC28A00A@qq.com (mailing list archive)
State Awaiting Upstream
Delegated to: Netdev Maintainers
Headers show
Series wifi: rtw88: error codes fix patches | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Zhang Shurong April 25, 2023, 4:24 p.m. UTC
If there is a failure during copy_from_user, rtw_debugfs_copy_from_user
should return negative error code instead of a positive value count.

Fix this bug by returning correct error code. Moreover, the check
of buffer against null is removed since it will be handled by
copy_from_user.

Signed-off-by: Zhang Shurong <zhang_shurong@foxmail.com>
---
 drivers/net/wireless/realtek/rtw88/debug.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Ping-Ke Shih April 26, 2023, 4:28 a.m. UTC | #1
> -----Original Message-----
> From: Zhang Shurong <zhang_shurong@foxmail.com>
> Sent: Wednesday, April 26, 2023 12:24 AM
> To: tony0620emma@gmail.com
> Cc: kvalo@kernel.org; davem@davemloft.net; edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> linux-wireless@vger.kernel.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Zhang Shurong
> <zhang_shurong@foxmail.com>
> Subject: [PATCH v2 1/2] wifi: rtw88: fix incorrect error codes in rtw_debugfs_copy_from_user
> 
> If there is a failure during copy_from_user, rtw_debugfs_copy_from_user
> should return negative error code instead of a positive value count.
> 
> Fix this bug by returning correct error code. Moreover, the check
> of buffer against null is removed since it will be handled by
> copy_from_user.
> 
> Signed-off-by: Zhang Shurong <zhang_shurong@foxmail.com>

Reviewed-by: Ping-Ke Shih <pkshih@realtek.com>

> ---
>  drivers/net/wireless/realtek/rtw88/debug.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/debug.c b/drivers/net/wireless/realtek/rtw88/debug.c
> index fa3d73b333ba..3da477e1ebd3 100644
> --- a/drivers/net/wireless/realtek/rtw88/debug.c
> +++ b/drivers/net/wireless/realtek/rtw88/debug.c
> @@ -183,8 +183,8 @@ static int rtw_debugfs_copy_from_user(char tmp[], int size,
> 
>         tmp_len = (count > size - 1 ? size - 1 : count);
> 
> -       if (!buffer || copy_from_user(tmp, buffer, tmp_len))
> -               return count;
> +       if (copy_from_user(tmp, buffer, tmp_len))
> +               return -EFAULT;
> 
>         tmp[tmp_len] = '\0';
> 
> --
> 2.40.0
Ping-Ke Shih April 26, 2023, 4:38 a.m. UTC | #2
> -----Original Message-----
> From: Ping-Ke Shih <pkshih@realtek.com>
> Sent: Wednesday, April 26, 2023 12:29 PM
> To: Zhang Shurong <zhang_shurong@foxmail.com>; tony0620emma@gmail.com
> Cc: kvalo@kernel.org; davem@davemloft.net; edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> linux-wireless@vger.kernel.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH v2 1/2] wifi: rtw88: fix incorrect error codes in rtw_debugfs_copy_from_user
> 
> > -----Original Message-----
> > From: Zhang Shurong <zhang_shurong@foxmail.com>
> > Sent: Wednesday, April 26, 2023 12:24 AM
> > To: tony0620emma@gmail.com
> > Cc: kvalo@kernel.org; davem@davemloft.net; edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> > linux-wireless@vger.kernel.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Zhang Shurong
> > <zhang_shurong@foxmail.com>
> > Subject: [PATCH v2 1/2] wifi: rtw88: fix incorrect error codes in rtw_debugfs_copy_from_user
> >
> > If there is a failure during copy_from_user, rtw_debugfs_copy_from_user
> > should return negative error code instead of a positive value count.
> >
> > Fix this bug by returning correct error code. Moreover, the check
> > of buffer against null is removed since it will be handled by
> > copy_from_user.
> >
> > Signed-off-by: Zhang Shurong <zhang_shurong@foxmail.com>
> 
> Reviewed-by: Ping-Ke Shih <pkshih@realtek.com>

I would take back this temporarily because of below. 

> 
> > ---
> >  drivers/net/wireless/realtek/rtw88/debug.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/wireless/realtek/rtw88/debug.c b/drivers/net/wireless/realtek/rtw88/debug.c
> > index fa3d73b333ba..3da477e1ebd3 100644
> > --- a/drivers/net/wireless/realtek/rtw88/debug.c
> > +++ b/drivers/net/wireless/realtek/rtw88/debug.c
> > @@ -183,8 +183,8 @@ static int rtw_debugfs_copy_from_user(char tmp[], int size,
> >
> >         tmp_len = (count > size - 1 ? size - 1 : count);
> >
> > -       if (!buffer || copy_from_user(tmp, buffer, tmp_len))
> > -               return count;
> > +       if (copy_from_user(tmp, buffer, tmp_len))
> > +               return -EFAULT;
> >
> >         tmp[tmp_len] = '\0';
> >

In the second patch, you check 'ret < 0' instead of 'ret'. That looks like
you can possibly return positive value (e.g. count), but actually only
return 0 or - EFAULT after this patch. So, I would like change first or second
patch to make them intuitive. 

return 0 or -EFAULT          --> check by if (ret)
return 0 or -EFAULT or count --> check by if (ret < 0)


+       ret = rtw_debugfs_copy_from_user(tmp, sizeof(tmp), buffer, count, 2);
+       if (ret < 0)
+               return ret;
diff mbox series

Patch

diff --git a/drivers/net/wireless/realtek/rtw88/debug.c b/drivers/net/wireless/realtek/rtw88/debug.c
index fa3d73b333ba..3da477e1ebd3 100644
--- a/drivers/net/wireless/realtek/rtw88/debug.c
+++ b/drivers/net/wireless/realtek/rtw88/debug.c
@@ -183,8 +183,8 @@  static int rtw_debugfs_copy_from_user(char tmp[], int size,
 
 	tmp_len = (count > size - 1 ? size - 1 : count);
 
-	if (!buffer || copy_from_user(tmp, buffer, tmp_len))
-		return count;
+	if (copy_from_user(tmp, buffer, tmp_len))
+		return -EFAULT;
 
 	tmp[tmp_len] = '\0';