Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Authenticated-By: 
From:   Ping-Ke Shih <pkshih@realtek.com>
To:     Ping-Ke Shih <pkshih@realtek.com>,
        Zhang Shurong <zhang_shurong@foxmail.com>,
        "tony0620emma@gmail.com" <tony0620emma@gmail.com>
CC:     "kvalo@kernel.org" <kvalo@kernel.org>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "edumazet@google.com" <edumazet@google.com>,
        "kuba@kernel.org" <kuba@kernel.org>,
        "pabeni@redhat.com" <pabeni@redhat.com>,
        "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@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
Thread-Topic: [PATCH v2 1/2] wifi: rtw88: fix incorrect error codes in
 rtw_debugfs_copy_from_user
Thread-Index: AQHZd5JwMo0jajhBZEqbv0oa4rkPjK88/+uggAABEWA=
Date:   Wed, 26 Apr 2023 04:38:33 +0000
Message-ID: <6fb7c4f35f634fb2a0d1afa8818dbf6e@realtek.com>
References: <cover.1682438257.git.zhang_shurong@foxmail.com>
 <tencent_8BA35E4B9CDF40D4AE6D8D831D7ACC28A00A@qq.com>
 <9c3ddb61020749d4811053e25c949b05@realtek.com>
In-Reply-To: <9c3ddb61020749d4811053e25c949b05@realtek.com>
Accept-Language: en-US, zh-TW
Content-Language: zh-TW
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
Precedence: bulk


> -----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;