Re: [PATCH 01/10] wifi: rtw88: fix incorrect error codes in rtw_debugfs_set_write_reg

[foxmail]

Thank you a lot for your kind reply, I will resend it as 2 patches.

> 2023年4月24日 09:58，Ping-Ke Shih <pkshih@xxxxxxxxxxx> 写道：
> 
>> -----Original Message-----
>> From: Zhang Shurong <zhang_shurong@xxxxxxxxxxx>
>> Sent: Saturday, April 22, 2023 6:05 PM
>> To: tony0620emma@xxxxxxxxx
>> Cc: kvalo@xxxxxxxxxx; davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx;
>> linux-wireless@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Zhang Shurong
>> <zhang_shurong@xxxxxxxxxxx>
>> 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@xxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxx>
>> Sent: Saturday, April 22, 2023 6:05 PM
>> To: tony0620emma@xxxxxxxxx
>> Cc: kvalo@xxxxxxxxxx; davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx;
>> linux-wireless@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Zhang Shurong
>> <zhang_shurong@xxxxxxxxxxx>
>> 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@xxxxxxxxxxx>
>> ---
>> 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