From: Xiaoke Wang <[email protected]>
kzalloc() is a memory allocation function which can return NULL when
some internal memory errors happen. So it is better to handle the return
of it to prevent potential wrong memory access.
For the kzalloc() in go_add_group_info_attr(), since there is a lack
of error handlers along the call chain it lies and the lifetime of
`pdata_attr` is only in go_add_group_info_attr(), `pdata_attr` is roughly
changed to a local variable on stack like the other functions in
rtw_p2p.c, such as `u8 p2pie[MAX_P2P_IE_LEN] = { 0x00 };` in
issue_p2p_presence_resp().
Signed-off-by: Xiaoke Wang <[email protected]>
---
drivers/staging/r8188eu/core/rtw_p2p.c | 6 ++----
drivers/staging/r8188eu/core/rtw_xmit.c | 12 +++++++++---
drivers/staging/r8188eu/include/rtw_xmit.h | 2 +-
3 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/r8188eu/core/rtw_p2p.c b/drivers/staging/r8188eu/core/rtw_p2p.c
index e2b6cf2..f1a5df8 100644
--- a/drivers/staging/r8188eu/core/rtw_p2p.c
+++ b/drivers/staging/r8188eu/core/rtw_p2p.c
@@ -27,15 +27,14 @@ static u32 go_add_group_info_attr(struct wifidirect_info *pwdinfo, u8 *pbuf)
struct list_head *phead, *plist;
u32 len = 0;
u16 attr_len = 0;
- u8 tmplen, *pdata_attr, *pstart, *pcur;
+ u8 pdata_attr[MAX_P2P_IE_LEN] = { 0x00 };
+ u8 tmplen, *pstart, *pcur;
struct sta_info *psta = NULL;
struct adapter *padapter = pwdinfo->padapter;
struct sta_priv *pstapriv = &padapter->stapriv;
DBG_88E("%s\n", __func__);
- pdata_attr = kzalloc(MAX_P2P_IE_LEN, GFP_KERNEL);
-
pstart = pdata_attr;
pcur = pdata_attr;
@@ -106,7 +105,6 @@ static u32 go_add_group_info_attr(struct wifidirect_info *pwdinfo, u8 *pbuf)
if (attr_len > 0)
len = rtw_set_p2p_attr_content(pbuf, P2P_ATTR_GROUP_INFO, attr_len, pdata_attr);
- kfree(pdata_attr);
return len;
}
diff --git a/drivers/staging/r8188eu/core/rtw_xmit.c b/drivers/staging/r8188eu/core/rtw_xmit.c
index 46fe62c..5888979 100644
--- a/drivers/staging/r8188eu/core/rtw_xmit.c
+++ b/drivers/staging/r8188eu/core/rtw_xmit.c
@@ -179,7 +179,9 @@ s32 _rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
pxmitpriv->free_xmit_extbuf_cnt = num_xmit_extbuf;
- rtw_alloc_hwxmits(padapter);
+ res = rtw_alloc_hwxmits(padapter);
+ if (res == _FAIL)
+ goto exit;
rtw_init_hwxmits(pxmitpriv->hwxmits, pxmitpriv->hwxmit_entry);
for (i = 0; i < 4; i++)
@@ -202,7 +204,6 @@ s32 _rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
rtw_hal_init_xmit_priv(padapter);
exit:
-
return res;
}
@@ -1516,7 +1517,7 @@ s32 rtw_xmit_classifier(struct adapter *padapter, struct xmit_frame *pxmitframe)
return res;
}
-void rtw_alloc_hwxmits(struct adapter *padapter)
+s32 rtw_alloc_hwxmits(struct adapter *padapter)
{
struct hw_xmit *hwxmits;
struct xmit_priv *pxmitpriv = &padapter->xmitpriv;
@@ -1525,6 +1526,9 @@ void rtw_alloc_hwxmits(struct adapter *padapter)
pxmitpriv->hwxmits = kzalloc(sizeof(struct hw_xmit) * pxmitpriv->hwxmit_entry, GFP_KERNEL);
+ if (!pxmitpriv->hwxmits)
+ return _FAIL;
+
hwxmits = pxmitpriv->hwxmits;
if (pxmitpriv->hwxmit_entry == 5) {
@@ -1540,6 +1544,8 @@ void rtw_alloc_hwxmits(struct adapter *padapter)
hwxmits[3] .sta_queue = &pxmitpriv->bk_pending;
} else {
}
+
+ return _SUCCESS;
}
void rtw_free_hwxmits(struct adapter *padapter)
diff --git a/drivers/staging/r8188eu/include/rtw_xmit.h b/drivers/staging/r8188eu/include/rtw_xmit.h
index 5f6e240..b45cd29 100644
--- a/drivers/staging/r8188eu/include/rtw_xmit.h
+++ b/drivers/staging/r8188eu/include/rtw_xmit.h
@@ -345,7 +345,7 @@ s32 rtw_txframes_sta_ac_pending(struct adapter *padapter,
void rtw_init_hwxmits(struct hw_xmit *phwxmit, int entry);
s32 _rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter);
void _rtw_free_xmit_priv(struct xmit_priv *pxmitpriv);
-void rtw_alloc_hwxmits(struct adapter *padapter);
+s32 rtw_alloc_hwxmits(struct adapter *padapter);
void rtw_free_hwxmits(struct adapter *padapter);
s32 rtw_xmit(struct adapter *padapter, struct sk_buff **pkt);
--
On Wed, Mar 30, 2022 at 11:16:07PM +0800, [email protected] wrote:
> From: Xiaoke Wang <[email protected]>
>
> kzalloc() is a memory allocation function which can return NULL when
> some internal memory errors happen. So it is better to handle the return
> of it to prevent potential wrong memory access.
> For the kzalloc() in go_add_group_info_attr(), since there is a lack
> of error handlers along the call chain it lies and the lifetime of
> `pdata_attr` is only in go_add_group_info_attr(), `pdata_attr` is roughly
> changed to a local variable on stack like the other functions in
> rtw_p2p.c, such as `u8 p2pie[MAX_P2P_IE_LEN] = { 0x00 };` in
> issue_p2p_presence_resp().
>
> Signed-off-by: Xiaoke Wang <[email protected]>
> ---
> drivers/staging/r8188eu/core/rtw_p2p.c | 6 ++----
> drivers/staging/r8188eu/core/rtw_xmit.c | 12 +++++++++---
> drivers/staging/r8188eu/include/rtw_xmit.h | 2 +-
> 3 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/core/rtw_p2p.c b/drivers/staging/r8188eu/core/rtw_p2p.c
> index e2b6cf2..f1a5df8 100644
> --- a/drivers/staging/r8188eu/core/rtw_p2p.c
> +++ b/drivers/staging/r8188eu/core/rtw_p2p.c
> @@ -27,15 +27,14 @@ static u32 go_add_group_info_attr(struct wifidirect_info *pwdinfo, u8 *pbuf)
> struct list_head *phead, *plist;
> u32 len = 0;
> u16 attr_len = 0;
> - u8 tmplen, *pdata_attr, *pstart, *pcur;
> + u8 pdata_attr[MAX_P2P_IE_LEN] = { 0x00 };
You just created a huge variable on the stack. Are you _SURE_ that is
ok?
Have you tested this change to make sure it works? If not, I can't take
it, sorry.
greg k-h
On Wed, Mar 30, 2022 at 11:16:07PM +0800, [email protected] wrote:
> diff --git a/drivers/staging/r8188eu/core/rtw_p2p.c b/drivers/staging/r8188eu/core/rtw_p2p.c
> index e2b6cf2..f1a5df8 100644
> --- a/drivers/staging/r8188eu/core/rtw_p2p.c
> +++ b/drivers/staging/r8188eu/core/rtw_p2p.c
> @@ -27,15 +27,14 @@ static u32 go_add_group_info_attr(struct wifidirect_info *pwdinfo, u8 *pbuf)
> struct list_head *phead, *plist;
> u32 len = 0;
> u16 attr_len = 0;
> - u8 tmplen, *pdata_attr, *pstart, *pcur;
> + u8 pdata_attr[MAX_P2P_IE_LEN] = { 0x00 };
> + u8 tmplen, *pstart, *pcur;
> struct sta_info *psta = NULL;
> struct adapter *padapter = pwdinfo->padapter;
> struct sta_priv *pstapriv = &padapter->stapriv;
>
> DBG_88E("%s\n", __func__);
>
> - pdata_attr = kzalloc(MAX_P2P_IE_LEN, GFP_KERNEL);
> -
> pstart = pdata_attr;
> pcur = pdata_attr;
>
> @@ -106,7 +105,6 @@ static u32 go_add_group_info_attr(struct wifidirect_info *pwdinfo, u8 *pbuf)
> if (attr_len > 0)
> len = rtw_set_p2p_attr_content(pbuf, P2P_ATTR_GROUP_INFO, attr_len, pdata_attr);
>
> - kfree(pdata_attr);
> return len;
> }
>
This part is fine. This change is can be pulled into a separate patch
and reviewed by itself.
> diff --git a/drivers/staging/r8188eu/core/rtw_xmit.c b/drivers/staging/r8188eu/core/rtw_xmit.c
> index 46fe62c..5888979 100644
> --- a/drivers/staging/r8188eu/core/rtw_xmit.c
> +++ b/drivers/staging/r8188eu/core/rtw_xmit.c
> @@ -179,7 +179,9 @@ s32 _rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
>
> pxmitpriv->free_xmit_extbuf_cnt = num_xmit_extbuf;
>
> - rtw_alloc_hwxmits(padapter);
> + res = rtw_alloc_hwxmits(padapter);
> + if (res == _FAIL)
> + goto exit;
There needs to be some cleanup if rtw_alloc_hwxmits() fails.
> rtw_init_hwxmits(pxmitpriv->hwxmits, pxmitpriv->hwxmit_entry);
>
> for (i = 0; i < 4; i++)
> @@ -202,7 +204,6 @@ s32 _rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
> rtw_hal_init_xmit_priv(padapter);
>
> exit:
> -
> return res;
This whitespace change has nothing to do with allocating memory. Leave
it out.
> }
>
> @@ -1516,7 +1517,7 @@ s32 rtw_xmit_classifier(struct adapter *padapter, struct xmit_frame *pxmitframe)
> return res;
> }
>
> -void rtw_alloc_hwxmits(struct adapter *padapter)
> +s32 rtw_alloc_hwxmits(struct adapter *padapter)
> {
> struct hw_xmit *hwxmits;
> struct xmit_priv *pxmitpriv = &padapter->xmitpriv;
> @@ -1525,6 +1526,9 @@ void rtw_alloc_hwxmits(struct adapter *padapter)
>
> pxmitpriv->hwxmits = kzalloc(sizeof(struct hw_xmit) * pxmitpriv->hwxmit_entry, GFP_KERNEL);
>
> + if (!pxmitpriv->hwxmits)
Don't leave a blank line between the allocation and the check.
> + return _FAIL;
> +
> hwxmits = pxmitpriv->hwxmits;
>
> if (pxmitpriv->hwxmit_entry == 5) {
regards,
dan carpenter