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

[Ping-Ke Shih]

> -----Original Message-----
> From: Ping-Ke Shih <pkshih@xxxxxxxxxxx>
> Sent: Wednesday, April 26, 2023 12:29 PM
> To: Zhang Shurong <zhang_shurong@xxxxxxxxxxx>; tony0620emma@xxxxxxxxx
> Cc: kvalo@xxxxxxxxxx; davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx;
> linux-wireless@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxx>
> > Sent: Wednesday, April 26, 2023 12:24 AM
> > 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 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@xxxxxxxxxxx>
> 
> Reviewed-by: Ping-Ke Shih <pkshih@xxxxxxxxxxx>

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;