linux-kernel - RE: [PATCH v2 1/2] wifi: rtw88: fix incorrect error codes in rtw_debugfs_copy_from

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <6fb7c4f35f634fb2a0d1afa8818dbf6e@realtek.com>
Date:   Wed, 26 Apr 2023 04:38:33 +0000
From:   Ping-Ke Shih <pkshih@...ltek.com>
To:     Ping-Ke Shih <pkshih@...ltek.com>,
        Zhang Shurong <zhang_shurong@...mail.com>,
        "tony0620emma@...il.com" <tony0620emma@...il.com>
CC:     "kvalo@...nel.org" <kvalo@...nel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2 1/2] wifi: rtw88: fix incorrect error codes in rtw_debugfs_copy_from_user



> -----Original Message-----
> From: Ping-Ke Shih <pkshih@...ltek.com>
> Sent: Wednesday, April 26, 2023 12:29 PM
> To: Zhang Shurong <zhang_shurong@...mail.com>; tony0620emma@...il.com
> Cc: kvalo@...nel.org; davem@...emloft.net; edumazet@...gle.com; kuba@...nel.org; pabeni@...hat.com;
> linux-wireless@...r.kernel.org; netdev@...r.kernel.org; linux-kernel@...r.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@...mail.com>
> > Sent: Wednesday, April 26, 2023 12:24 AM
> > To: tony0620emma@...il.com
> > Cc: kvalo@...nel.org; davem@...emloft.net; edumazet@...gle.com; kuba@...nel.org; pabeni@...hat.com;
> > linux-wireless@...r.kernel.org; netdev@...r.kernel.org; linux-kernel@...r.kernel.org; Zhang Shurong
> > <zhang_shurong@...mail.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@...mail.com>
> 
> Reviewed-by: Ping-Ke Shih <pkshih@...ltek.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;