| Message ID | tencent_6E21370EB57D5B7060611EB60A96A88B1208@qq.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [01/10] wifi: rtw88: fix incorrect error codes in rtw_debugfs_set_write_reg | expand |
> -----Original Message----- > From: Zhang Shurong <zhang_shurong@foxmail.com> > Sent: Saturday, April 22, 2023 6:05 PM > 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 01/10] wifi: rtw88: fix incorrect error codes in rtw_debugfs_set_write_reg > > If there is a failure during copy_from_user or user-provided data > buffer is invalid, rtw_debugfs_set_write_reg 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 | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/wireless/realtek/rtw88/debug.c b/drivers/net/wireless/realtek/rtw88/debug.c > index fa3d73b333ba..bc41c5a7acaf 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; This patchset is fine to me. The only thing is this chunk can be first patch, and squash other patches to second patch because they do the same thing in the same driver. > > tmp[tmp_len] = '\0'; > > @@ -338,14 +338,17 @@ static ssize_t rtw_debugfs_set_write_reg(struct file *filp, > char tmp[32 + 1]; > u32 addr, val, len; > int num; > + int ret; > > - rtw_debugfs_copy_from_user(tmp, sizeof(tmp), buffer, count, 3); > + ret = rtw_debugfs_copy_from_user(tmp, sizeof(tmp), buffer, count, 3); > + if (ret < 0) > + return ret; > > /* write BB/MAC register */ > num = sscanf(tmp, "%x %x %x", &addr, &val, &len); > > if (num != 3) > - return count; > + return -EINVAL; > > switch (len) { > case 1: > -- > 2.40.0
Thank you a lot for your kind reply, I will resend it as 2 patches. > 2023年4月24日 09:58，Ping-Ke Shih <pkshih@realtek.com> 写道： > >> -----Original Message----- >> From: Zhang Shurong <zhang_shurong@foxmail.com> >> Sent: Saturday, April 22, 2023 6:05 PM >> 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 01/10] wifi: rtw88: fix incorrect error codes in rtw_debugfs_set_write_reg >> >> If there is a failure during copy_from_user or user-provided data >> buffer is invalid, rtw_debugfs_set_write_reg 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 | 11 +++++++---- >> 1 file changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/net/wireless/realtek/rtw88/debug.c b/drivers/net/wireless/realtek/rtw88/debug.c >> index fa3d73b333ba..bc41c5a7acaf 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; > > This patchset is fine to me. The only thing is this chunk can be first patch, > and squash other patches to second patch because they do the same thing > in the same driver. > > >> >> tmp[tmp_len] = '\0'; >> >> @@ -338,14 +338,17 @@ static ssize_t rtw_debugfs_set_write_reg(struct file *filp, >> char tmp[32 + 1]; >> u32 addr, val, len; >> int num; >> + int ret; >> >> - rtw_debugfs_copy_from_user(tmp, sizeof(tmp), buffer, count, 3); >> + ret = rtw_debugfs_copy_from_user(tmp, sizeof(tmp), buffer, count, 3); >> + if (ret < 0) >> + return ret; >> >> /* write BB/MAC register */ >> num = sscanf(tmp, "%x %x %x", &addr, &val, &len); >> >> if (num != 3) >> - return count; >> + return -EINVAL; >> >> switch (len) { >> case 1: >> -- >> 2.40.0 > >> -----Original Message----- >> From: Zhang Shurong <zhang_shurong@foxmail.com> >> Sent: Saturday, April 22, 2023 6:05 PM >> 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 01/10] wifi: rtw88: fix incorrect error codes in rtw_debugfs_set_write_reg >> >> If there is a failure during copy_from_user or user-provided data >> buffer is invalid, rtw_debugfs_set_write_reg 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 | 11 +++++++---- >> 1 file changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/net/wireless/realtek/rtw88/debug.c b/drivers/net/wireless/realtek/rtw88/debug.c >> index fa3d73b333ba..bc41c5a7acaf 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; > > This patchset is fine to me. The only thing is this chunk can be first patch, > and squash other patches to second patch because they do the same thing > in the same driver. > > >> >> tmp[tmp_len] = '\0'; >> >> @@ -338,14 +338,17 @@ static ssize_t rtw_debugfs_set_write_reg(struct file *filp, >> char tmp[32 + 1]; >> u32 addr, val, len; >> int num; >> + int ret; >> >> - rtw_debugfs_copy_from_user(tmp, sizeof(tmp), buffer, count, 3); >> + ret = rtw_debugfs_copy_from_user(tmp, sizeof(tmp), buffer, count, 3); >> + if (ret < 0) >> + return ret; >> >> /* write BB/MAC register */ >> num = sscanf(tmp, "%x %x %x", &addr, &val, &len); >> >> if (num != 3) >> - return count; >> + return -EINVAL; >> >> switch (len) { >> case 1: >> -- >> 2.40.0
diff --git a/drivers/net/wireless/realtek/rtw88/debug.c b/drivers/net/wireless/realtek/rtw88/debug.c index fa3d73b333ba..bc41c5a7acaf 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'; @@ -338,14 +338,17 @@ static ssize_t rtw_debugfs_set_write_reg(struct file *filp, char tmp[32 + 1]; u32 addr, val, len; int num; + int ret; - rtw_debugfs_copy_from_user(tmp, sizeof(tmp), buffer, count, 3); + ret = rtw_debugfs_copy_from_user(tmp, sizeof(tmp), buffer, count, 3); + if (ret < 0) + return ret; /* write BB/MAC register */ num = sscanf(tmp, "%x %x %x", &addr, &val, &len); if (num != 3) - return count; + return -EINVAL; switch (len) { case 1:
If there is a failure during copy_from_user or user-provided data buffer is invalid, rtw_debugfs_set_write_reg 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 | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)