kzalloc may fails, pdata_attr might be null and will cause
illegal address access later.
Signed-off-by: Kang Chen <[email protected]>
---
drivers/staging/r8188eu/core/rtw_p2p.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/staging/r8188eu/core/rtw_p2p.c b/drivers/staging/r8188eu/core/rtw_p2p.c
index 93d3c9c43..802e1170a 100644
--- a/drivers/staging/r8188eu/core/rtw_p2p.c
+++ b/drivers/staging/r8188eu/core/rtw_p2p.c
@@ -31,6 +31,8 @@ static u32 go_add_group_info_attr(struct wifidirect_info *pwdinfo, u8 *pbuf)
struct sta_priv *pstapriv = &padapter->stapriv;
pdata_attr = kzalloc(MAX_P2P_IE_LEN, GFP_KERNEL);
+ if (!pdata_attr)
+ return 0;
pstart = pdata_attr;
pcur = pdata_attr;
--
2.34.1
On Sun, Feb 26, 2023 at 09:25:00PM +0800, Kang Chen wrote:
> kzalloc may fails, pdata_attr might be null and will cause
> illegal address access later.
>
> Signed-off-by: Kang Chen <[email protected]>
> ---
> drivers/staging/r8188eu/core/rtw_p2p.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/staging/r8188eu/core/rtw_p2p.c b/drivers/staging/r8188eu/core/rtw_p2p.c
> index 93d3c9c43..802e1170a 100644
> --- a/drivers/staging/r8188eu/core/rtw_p2p.c
> +++ b/drivers/staging/r8188eu/core/rtw_p2p.c
> @@ -31,6 +31,8 @@ static u32 go_add_group_info_attr(struct wifidirect_info *pwdinfo, u8 *pbuf)
> struct sta_priv *pstapriv = &padapter->stapriv;
>
> pdata_attr = kzalloc(MAX_P2P_IE_LEN, GFP_KERNEL);
> + if (!pdata_attr)
> + return 0;
Return success here is not a good thing. We have to fix the caller to
check for errors. (Fixing a bug half way just makes it harder to find
the bug so it makes the situation worse).
regards,
dan carpenter
Hi, Dan,
Thanks for your review.
I noticed there is no error handling in the origin design (this call chain).
go_add_group_info_attr returns a len-like value indicating the length
of pbuf.
I don't think throwing an error to the caller is a good idea, the caller
doesn't seem to care about it.
So inserting a netdev_dbg or pr_debug here might be enough.
Do you have a better idea?
Best regards,
Kang Chen
On Mon, Feb 27, 2023 at 12:47 PM Dan Carpenter <[email protected]> wrote:
>
> On Sun, Feb 26, 2023 at 09:25:00PM +0800, Kang Chen wrote:
> > kzalloc may fails, pdata_attr might be null and will cause
> > illegal address access later.
> >
> > Signed-off-by: Kang Chen <[email protected]>
> > ---
> > drivers/staging/r8188eu/core/rtw_p2p.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/staging/r8188eu/core/rtw_p2p.c b/drivers/staging/r8188eu/core/rtw_p2p.c
> > index 93d3c9c43..802e1170a 100644
> > --- a/drivers/staging/r8188eu/core/rtw_p2p.c
> > +++ b/drivers/staging/r8188eu/core/rtw_p2p.c
> > @@ -31,6 +31,8 @@ static u32 go_add_group_info_attr(struct wifidirect_info *pwdinfo, u8 *pbuf)
> > struct sta_priv *pstapriv = &padapter->stapriv;
> >
> > pdata_attr = kzalloc(MAX_P2P_IE_LEN, GFP_KERNEL);
> > + if (!pdata_attr)
> > + return 0;
>
> Return success here is not a good thing. We have to fix the caller to
> check for errors. (Fixing a bug half way just makes it harder to find
> the bug so it makes the situation worse).
>
> regards,
> dan carpenter
>
On Mon, Feb 27, 2023 at 03:11:21PM +0800, Kang Chen wrote:
> Hi, Dan,
>
> Thanks for your review.
> I noticed there is no error handling in the origin design (this call chain).
> go_add_group_info_attr returns a len-like value indicating the length
> of pbuf.
> I don't think throwing an error to the caller is a good idea, the caller
> doesn't seem to care about it.
> So inserting a netdev_dbg or pr_debug here might be enough.
> Do you have a better idea?
As I mentioned in my email, we need to fix the caller to care about it.
regards,
dan carpenter
>
> Best regards,
> Kang Chen
>
>
> On Mon, Feb 27, 2023 at 12:47 PM Dan Carpenter <[email protected]> wrote:
> >
> > On Sun, Feb 26, 2023 at 09:25:00PM +0800, Kang Chen wrote:
> > > kzalloc may fails, pdata_attr might be null and will cause
> > > illegal address access later.
> > >
> > > Signed-off-by: Kang Chen <[email protected]>
> > > ---
> > > drivers/staging/r8188eu/core/rtw_p2p.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/staging/r8188eu/core/rtw_p2p.c b/drivers/staging/r8188eu/core/rtw_p2p.c
> > > index 93d3c9c43..802e1170a 100644
> > > --- a/drivers/staging/r8188eu/core/rtw_p2p.c
> > > +++ b/drivers/staging/r8188eu/core/rtw_p2p.c
> > > @@ -31,6 +31,8 @@ static u32 go_add_group_info_attr(struct wifidirect_info *pwdinfo, u8 *pbuf)
> > > struct sta_priv *pstapriv = &padapter->stapriv;
> > >
> > > pdata_attr = kzalloc(MAX_P2P_IE_LEN, GFP_KERNEL);
> > > + if (!pdata_attr)
> > > + return 0;
> >
> > Return success here is not a good thing. We have to fix the caller to
> > check for errors. (Fixing a bug half way just makes it harder to find
> > the bug so it makes the situation worse).
> >
> > regards,
> > dan carpenter
> >
On Mon, Feb 27, 2023 at 03:11:21PM +0800, Kang Chen wrote:
> Hi, Dan,
>
> Thanks for your review.
> I noticed there is no error handling in the origin design (this call chain).
> go_add_group_info_attr returns a len-like value indicating the length
> of pbuf.
> I don't think throwing an error to the caller is a good idea, the caller
> doesn't seem to care about it.
> So inserting a netdev_dbg or pr_debug here might be enough.
> Do you have a better idea?
>
The bug is real, yes. But you have your static checker which can detect
it and I also have an unpublished static checker test which detects this
bug.
drivers/staging/r8188eu/core/rtw_p2p.c:106 go_add_group_info_attr() warn: 'pdata_attr' was never checked for NULL
If we just hide the bug enough for so that the static checker cannot
find the bug then we're taking a step backward. When this driver is
ready to leave staging then normally I review every static checker
warning. But if we hide the warning then it will never be fixed.
regards,
dan carpenter