* [PATCH] staging: r8188eu: check the return value of kzalloc()
@ 2022-03-25 6:53 xkernel.wang
2022-03-29 15:57 ` Greg KH
0 siblings, 1 reply; 4+ messages in thread
From: xkernel.wang @ 2022-03-25 6:53 UTC (permalink / raw)
To: Larry.Finger, phil, gregkh; +Cc: linux-staging, linux-kernel, Xiaoke Wang
From: Xiaoke Wang <xkernel.wang@foxmail.com>
kzalloc() is a memory allocation function which can return NULL when
some internal memory errors happen. So it is better to check the return
of it to prevent potential wrong memory access.
Signed-off-by: Xiaoke Wang <xkernel.wang@foxmail.com>
---
drivers/staging/r8188eu/core/rtw_p2p.c | 2 ++
drivers/staging/r8188eu/core/rtw_xmit.c | 6 ++++++
2 files changed, 8 insertions(+)
diff --git a/drivers/staging/r8188eu/core/rtw_p2p.c b/drivers/staging/r8188eu/core/rtw_p2p.c
index e2b6cf2..503c4a5 100644
--- a/drivers/staging/r8188eu/core/rtw_p2p.c
+++ b/drivers/staging/r8188eu/core/rtw_p2p.c
@@ -35,6 +35,8 @@ static u32 go_add_group_info_attr(struct wifidirect_info *pwdinfo, u8 *pbuf)
DBG_88E("%s\n", __func__);
pdata_attr = kzalloc(MAX_P2P_IE_LEN, GFP_KERNEL);
+ if (!pdata_attr)
+ return 0;
pstart = pdata_attr;
pcur = pdata_attr;
diff --git a/drivers/staging/r8188eu/core/rtw_xmit.c b/drivers/staging/r8188eu/core/rtw_xmit.c
index 46fe62c..1696272 100644
--- a/drivers/staging/r8188eu/core/rtw_xmit.c
+++ b/drivers/staging/r8188eu/core/rtw_xmit.c
@@ -180,6 +180,10 @@ s32 _rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
pxmitpriv->free_xmit_extbuf_cnt = num_xmit_extbuf;
rtw_alloc_hwxmits(padapter);
+ if (!pxmitpriv->hwxmits) {
+ res = _FAIL;
+ goto exit;
+ }
rtw_init_hwxmits(pxmitpriv->hwxmits, pxmitpriv->hwxmit_entry);
for (i = 0; i < 4; i++)
@@ -1524,6 +1528,8 @@ void rtw_alloc_hwxmits(struct adapter *padapter)
pxmitpriv->hwxmit_entry = HWXMIT_ENTRY;
pxmitpriv->hwxmits = kzalloc(sizeof(struct hw_xmit) * pxmitpriv->hwxmit_entry, GFP_KERNEL);
+ if (!pxmitpriv->hwxmits)
+ return;
hwxmits = pxmitpriv->hwxmits;
--
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] staging: r8188eu: check the return value of kzalloc()
2022-03-25 6:53 [PATCH] staging: r8188eu: check the return value of kzalloc() xkernel.wang
@ 2022-03-29 15:57 ` Greg KH
2022-03-29 16:53 ` Xiaoke Wang
2022-03-30 7:34 ` Xiaoke Wang
0 siblings, 2 replies; 4+ messages in thread
From: Greg KH @ 2022-03-29 15:57 UTC (permalink / raw)
To: xkernel.wang; +Cc: Larry.Finger, phil, linux-staging, linux-kernel
On Fri, Mar 25, 2022 at 02:53:30PM +0800, xkernel.wang@foxmail.com wrote:
> From: Xiaoke Wang <xkernel.wang@foxmail.com>
>
> kzalloc() is a memory allocation function which can return NULL when
> some internal memory errors happen. So it is better to check the return
> of it to prevent potential wrong memory access.
>
> Signed-off-by: Xiaoke Wang <xkernel.wang@foxmail.com>
> ---
> drivers/staging/r8188eu/core/rtw_p2p.c | 2 ++
> drivers/staging/r8188eu/core/rtw_xmit.c | 6 ++++++
> 2 files changed, 8 insertions(+)
>
> diff --git a/drivers/staging/r8188eu/core/rtw_p2p.c b/drivers/staging/r8188eu/core/rtw_p2p.c
> index e2b6cf2..503c4a5 100644
> --- a/drivers/staging/r8188eu/core/rtw_p2p.c
> +++ b/drivers/staging/r8188eu/core/rtw_p2p.c
> @@ -35,6 +35,8 @@ static u32 go_add_group_info_attr(struct wifidirect_info *pwdinfo, u8 *pbuf)
> DBG_88E("%s\n", __func__);
>
> pdata_attr = kzalloc(MAX_P2P_IE_LEN, GFP_KERNEL);
> + if (!pdata_attr)
> + return 0;
0 is not an error. Please propagate this error backwards properly.
>
> pstart = pdata_attr;
> pcur = pdata_attr;
> diff --git a/drivers/staging/r8188eu/core/rtw_xmit.c b/drivers/staging/r8188eu/core/rtw_xmit.c
> index 46fe62c..1696272 100644
> --- a/drivers/staging/r8188eu/core/rtw_xmit.c
> +++ b/drivers/staging/r8188eu/core/rtw_xmit.c
> @@ -180,6 +180,10 @@ s32 _rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
> pxmitpriv->free_xmit_extbuf_cnt = num_xmit_extbuf;
>
> rtw_alloc_hwxmits(padapter);
> + if (!pxmitpriv->hwxmits) {
> + res = _FAIL;
> + goto exit;
> + }
You just leaked memory resources :(
How did you test this?
> rtw_init_hwxmits(pxmitpriv->hwxmits, pxmitpriv->hwxmit_entry);
>
> for (i = 0; i < 4; i++)
> @@ -1524,6 +1528,8 @@ void rtw_alloc_hwxmits(struct adapter *padapter)
> pxmitpriv->hwxmit_entry = HWXMIT_ENTRY;
>
> pxmitpriv->hwxmits = kzalloc(sizeof(struct hw_xmit) * pxmitpriv->hwxmit_entry, GFP_KERNEL);
> + if (!pxmitpriv->hwxmits)
> + return;
You have to return an error, you can not keep going as if all is well.
Please always be VERY careful with these types of fixes. Especially if
you have not tested them.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Re: [PATCH] staging: r8188eu: check the return value of kzalloc()
2022-03-29 15:57 ` Greg KH
@ 2022-03-29 16:53 ` Xiaoke Wang
2022-03-30 7:34 ` Xiaoke Wang
1 sibling, 0 replies; 4+ messages in thread
From: Xiaoke Wang @ 2022-03-29 16:53 UTC (permalink / raw)
To: gregkh; +Cc: larry.finger, phil, linux-staging, linux-kernel
On Tue, 29 Mar 2022 23:57:10 +0800, gregkh@linuxfoundation.org wrote:
>> pdata_attr = kzalloc(MAX_P2P_IE_LEN, GFP_KERNEL);
>> + if (!pdata_attr)
>> + return 0;
>
> 0 is not an error. Please propagate this error backwards properly.
Ok. But the function go_add_group_info_attr() is called by:
> p2pielen += go_add_group_info_attr(pwdinfo, p2pie + p2pielen);
I am not sure how to properly handle it before, so I just deal with it
lazily to reduce unintentional modification.
I will make it a negative code in next version, but there is a lack of
proper error handlers along the call chain.
>> --- a/drivers/staging/r8188eu/core/rtw_xmit.c
>> +++ b/drivers/staging/r8188eu/core/rtw_xmit.c
>> @@ -180,6 +180,10 @@ s32 _rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
>> pxmitpriv->free_xmit_extbuf_cnt = num_xmit_extbuf;
>>
>> rtw_alloc_hwxmits(padapter);
>> + if (!pxmitpriv->hwxmits) {
>> + res = _FAIL;
>> + goto exit;
>> + }
>
> You just leaked memory resources :(
> How did you test this?
Emmm, I do not think this will leak as `pxmitpriv` or `padapter` are all
arguments from the caller. And this is similar to the same function in
drivers\staging\rtl8723bs\core\rtw_xmit.c.
>> @@ -1524,6 +1528,8 @@ void rtw_alloc_hwxmits(struct adapter *padapter)
>> pxmitpriv->hwxmit_entry = HWXMIT_ENTRY;
>>
>> pxmitpriv->hwxmits = kzalloc(sizeof(struct hw_xmit) * pxmitpriv->hwxmit_entry, GFP_KERNEL);
>> + if (!pxmitpriv->hwxmits)
>> + return;
>
> You have to return an error, you can not keep going as if all is well.
Ok, then I will change it to the shape like s32 _rtw_init_xmit_priv()
in drivers\staging\rtl8723bs\core\rtw_xmit.c and return `_FAIL`.
>
> Please always be VERY careful with these types of fixes. Especially if
> you have not tested them.
Thank you for your suggestion, I will try my best ^_^.
Regards,
Xiaoke Wang
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Re: [PATCH] staging: r8188eu: check the return value of kzalloc()
2022-03-29 15:57 ` Greg KH
2022-03-29 16:53 ` Xiaoke Wang
@ 2022-03-30 7:34 ` Xiaoke Wang
1 sibling, 0 replies; 4+ messages in thread
From: Xiaoke Wang @ 2022-03-30 7:34 UTC (permalink / raw)
To: gregkh; +Cc: larry.finger, phil, linux-staging, linux-kernel
On Tue, 29 Mar 2022 23:57:10 +0800, gregkh@linuxfoundation.org wrote:
>> --- a/drivers/staging/r8188eu/core/rtw_xmit.c
>> +++ b/drivers/staging/r8188eu/core/rtw_xmit.c
>> @@ -180,6 +180,10 @@ s32 _rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
>> pxmitpriv->free_xmit_extbuf_cnt = num_xmit_extbuf;
>>
>> rtw_alloc_hwxmits(padapter);
>> + if (!pxmitpriv->hwxmits) {
>> + res = _FAIL;
>> + goto exit;
>> + }
>
> You just leaked memory resources :(
>
Sorry, you are right. I tracked the callers of _rtw_init_xmit_priv() and
found that they do not properly release the resources allocated by this
funciton. This means that the other error paths can also bring memory
leaks, as well as the same function in
drivers/staging/rtl8723bs/core/rtw_xmit.c.
There are also several similar issuses of alloc_hwxmits() and
_r8712_init_xmit_priv() in drivers/staging/rtl8712/rtl871x_xmit.c.
I will try to carefully fix them.
Regards,
Xiaoke Wang
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-03-30 7:34 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-25 6:53 [PATCH] staging: r8188eu: check the return value of kzalloc() xkernel.wang
2022-03-29 15:57 ` Greg KH
2022-03-29 16:53 ` Xiaoke Wang
2022-03-30 7:34 ` Xiaoke Wang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.