[PATCH v3] staging: r8188eu: add check for kzalloc

[PATCH v3] staging: r8188eu: add check for kzalloc

[Jiasheng Jiang]

As kzalloc() may return null pointer, it should be better to
check the return value and return error if fails in order
to avoid dereference of null pointer.
Moreover, the return value of rtw_alloc_hwxmits() should also
be dealt with.

Fixes: 15865124feed ("staging: r8188eu: introduce new core dir for RTL8188eu driver")
Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
---
Changelog

v1 -> v2:

*Change 1. Make rtw_alloc_hwxmits() return -ENOMEM on failure
and zero on success.

v2 -> v3

*Change 1. Add "res = _FAIL".
---
 drivers/staging/r8188eu/core/rtw_xmit.c    | 13 +++++++++++--
 drivers/staging/r8188eu/include/rtw_xmit.h |  2 +-
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_xmit.c b/drivers/staging/r8188eu/core/rtw_xmit.c
index c2a550e7250e..2ee92bbe66a0 100644
--- a/drivers/staging/r8188eu/core/rtw_xmit.c
+++ b/drivers/staging/r8188eu/core/rtw_xmit.c
@@ -178,7 +178,12 @@ 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) {
+		res = _FAIL;
+		goto exit;
+	}
+
 	rtw_init_hwxmits(pxmitpriv->hwxmits, pxmitpriv->hwxmit_entry);
 
 	for (i = 0; i < 4; i++)
@@ -1474,7 +1479,7 @@ s32 rtw_xmit_classifier(struct adapter *padapter, struct xmit_frame *pxmitframe)
 	return res;
 }
 
-void rtw_alloc_hwxmits(struct adapter *padapter)
+int rtw_alloc_hwxmits(struct adapter *padapter)
 {
 	struct hw_xmit *hwxmits;
 	struct xmit_priv *pxmitpriv = &padapter->xmitpriv;
@@ -1482,6 +1487,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 -ENOMEM;
 
 	hwxmits = pxmitpriv->hwxmits;
 
@@ -1498,6 +1505,8 @@ void rtw_alloc_hwxmits(struct adapter *padapter)
 		hwxmits[3] .sta_queue = &pxmitpriv->bk_pending;
 	} else {
 	}
+
+	return 0;
 }
 
 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 b2df1480d66b..e73632972900 100644
--- a/drivers/staging/r8188eu/include/rtw_xmit.h
+++ b/drivers/staging/r8188eu/include/rtw_xmit.h
@@ -341,7 +341,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);
+int rtw_alloc_hwxmits(struct adapter *padapter);
 void rtw_free_hwxmits(struct adapter *padapter);
 s32 rtw_xmit(struct adapter *padapter, struct sk_buff **pkt);
 
-- 
2.25.1

Re: [PATCH v3] staging: r8188eu: add check for kzalloc

[Dan Carpenter]

On Wed, May 18, 2022 at 03:59:57PM +0800, Jiasheng Jiang wrote:
> As kzalloc() may return null pointer, it should be better to
> check the return value and return error if fails in order
> to avoid dereference of null pointer.
> Moreover, the return value of rtw_alloc_hwxmits() should also
> be dealt with.
> 
> Fixes: 15865124feed ("staging: r8188eu: introduce new core dir for RTL8188eu driver")
> Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>

Looks good!

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter

Re: [PATCH v3] staging: r8188eu: add check for kzalloc

[Martin Kaiser]

Thus wrote Jiasheng Jiang (jiasheng@iscas.ac.cn):

> As kzalloc() may return null pointer, it should be better to
> check the return value and return error if fails in order
> to avoid dereference of null pointer.
> Moreover, the return value of rtw_alloc_hwxmits() should also
> be dealt with.

> Fixes: 15865124feed ("staging: r8188eu: introduce new core dir for RTL8188eu driver")
> Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
> ---
> Changelog

> v1 -> v2:

> *Change 1. Make rtw_alloc_hwxmits() return -ENOMEM on failure
> and zero on success.

> v2 -> v3

> *Change 1. Add "res = _FAIL".
> ---
>  drivers/staging/r8188eu/core/rtw_xmit.c    | 13 +++++++++++--
>  drivers/staging/r8188eu/include/rtw_xmit.h |  2 +-
>  2 files changed, 12 insertions(+), 3 deletions(-)

> diff --git a/drivers/staging/r8188eu/core/rtw_xmit.c b/drivers/staging/r8188eu/core/rtw_xmit.c
> index c2a550e7250e..2ee92bbe66a0 100644
> --- a/drivers/staging/r8188eu/core/rtw_xmit.c
> +++ b/drivers/staging/r8188eu/core/rtw_xmit.c
> @@ -178,7 +178,12 @@ 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);

This commit introduces a regression.

res is now 0 if the allocation succeeds.

> +	if (res) {
> +		res = _FAIL;
> +		goto exit;
> +	}
> +
>  	rtw_init_hwxmits(pxmitpriv->hwxmits, pxmitpriv->hwxmit_entry);

>  	for (i = 0; i < 4; i++)
> @@ -1474,7 +1479,7 @@ s32 rtw_xmit_classifier(struct adapter *padapter, struct xmit_frame *pxmitframe)


res is still 0 here - but the caller of _rtw_init_xmit_priv compares
this return value with _SUCCESS (1) or _FAIL (0) and interprets it as
_FAIL.

[ 3893.464932] r8188eu: module is from the staging directory, the quality is unknown, you have been warned.
[ 3893.543204] Chip Version Info: CHIP_8188E_Normal_Chip_TSMC_D_CUT_1T1R_RomVer(0)
[ 3893.713123] EEPROM ID = 0x8129
[ 3893.719205] r8188eu 1-1.5:1.0: _rtw_init_xmit_priv failed
[ 3893.823102] usb 1-1.5: reset full-speed USB device number 4 using ci_hdrc
[ 3893.986936] usbcore: registered new interface driver r8188eu

>  	return res;
>  }

Re: [PATCH v3] staging: r8188eu: add check for kzalloc

[Pavel Skripkin]

Hi Martin,

On 5/21/22 18:50, Martin Kaiser wrote:
> 
>>  	for (i = 0; i < 4; i++)
>> @@ -1474,7 +1479,7 @@ s32 rtw_xmit_classifier(struct adapter *padapter, struct xmit_frame *pxmitframe)
> 
> 
> res is still 0 here - but the caller of _rtw_init_xmit_priv compares
> this return value with _SUCCESS (1) or _FAIL (0) and interprets it as
> _FAIL.
> 

I think, it's time to make

s/_SUCCESS/0/
s/_FAIL/-1

since developers from outside of staging are confused.
The main problem will be with functions that return an int (or s32).

Will take a look.



With regards,
Pavel Skripkin

Re: [PATCH v3] staging: r8188eu: add check for kzalloc

[Phillip Potter]

On Sat, May 21, 2022 at 11:26:55PM +0300, Pavel Skripkin wrote:
> Hi Martin,
> 
> On 5/21/22 18:50, Martin Kaiser wrote:
> > 
> > >  	for (i = 0; i < 4; i++)
> > > @@ -1474,7 +1479,7 @@ s32 rtw_xmit_classifier(struct adapter *padapter, struct xmit_frame *pxmitframe)
> > 
> > 
> > res is still 0 here - but the caller of _rtw_init_xmit_priv compares
> > this return value with _SUCCESS (1) or _FAIL (0) and interprets it as
> > _FAIL.
> > 
> 
> I think, it's time to make
> 
> s/_SUCCESS/0/
> s/_FAIL/-1
> 
> since developers from outside of staging are confused.
> The main problem will be with functions that return an int (or s32).
> 
> Will take a look.
> 
> 
> 
> With regards,
> Pavel Skripkin

Hi Pavel,

I agree with you totally - we should change these semantics to reflect
how the rest of the kernel generally does things. That said, that is a
bigger patch set and I noticed the driver was broken before I read this
thread, so I've submitted a patch already just to fix the breakage for
now.

Changing these semantics is a bigger patch/patchset and I wanted to get
this out in the meantime - if you are looking at doing this conversion I
will by all means leave that alone as no desire to tread on anyones
toes :-)

Regards,
Phil

Re: [PATCH v3] staging: r8188eu: add check for kzalloc

[Pavel Skripkin]

Hi Phillip,

On 5/21/22 23:56, Phillip Potter wrote:
> Hi Pavel,
> 
> I agree with you totally - we should change these semantics to reflect
> how the rest of the kernel generally does things. That said, that is a
> bigger patch set and I noticed the driver was broken before I read this
> thread, so I've submitted a patch already just to fix the breakage for
> now.
> 
> Changing these semantics is a bigger patch/patchset and I wanted to get
> this out in the meantime - if you are looking at doing this conversion I
> will by all means leave that alone as no desire to tread on anyones
> toes :-)
> 

I think, redoing more then 500 place will take quite a long time, but 
driver should be fixed ASAP.

So let's apply your patch and then someone (maybe me) will cook a patch.




With regards,
Pavel Skripkin

Re: [PATCH v3] staging: r8188eu: add check for kzalloc

[Larry Finger]

On 5/21/22 15:26, Pavel Skripkin wrote:
> Hi Martin,
> 
> On 5/21/22 18:50, Martin Kaiser wrote:
>>
>>>      for (i = 0; i < 4; i++)
>>> @@ -1474,7 +1479,7 @@ s32 rtw_xmit_classifier(struct adapter *padapter, 
>>> struct xmit_frame *pxmitframe)
>>
>>
>> res is still 0 here - but the caller of _rtw_init_xmit_priv compares
>> this return value with _SUCCESS (1) or _FAIL (0) and interprets it as
>> _FAIL.
>>
> 
> I think, it's time to make
> 
> s/_SUCCESS/0/
> s/_FAIL/-1
> 
> since developers from outside of staging are confused.
> The main problem will be with functions that return an int (or s32).
> 
> Will take a look.

I agree; however, this change will likely break a lot of pending patches.


@GregKH: Could you apply all pending patches in preparation for this patch? If 
so, then Pavel could make this transformation while the list is relatively idle.

Larry

Re: [PATCH v3] staging: r8188eu: add check for kzalloc

[Greg KH]

On Sat, May 21, 2022 at 04:57:17PM -0500, Larry Finger wrote:
> On 5/21/22 15:26, Pavel Skripkin wrote:
> > Hi Martin,
> > 
> > On 5/21/22 18:50, Martin Kaiser wrote:
> > > 
> > > >      for (i = 0; i < 4; i++)
> > > > @@ -1474,7 +1479,7 @@ s32 rtw_xmit_classifier(struct adapter
> > > > *padapter, struct xmit_frame *pxmitframe)
> > > 
> > > 
> > > res is still 0 here - but the caller of _rtw_init_xmit_priv compares
> > > this return value with _SUCCESS (1) or _FAIL (0) and interprets it as
> > > _FAIL.
> > > 
> > 
> > I think, it's time to make
> > 
> > s/_SUCCESS/0/
> > s/_FAIL/-1
> > 
> > since developers from outside of staging are confused.
> > The main problem will be with functions that return an int (or s32).
> > 
> > Will take a look.
> 
> I agree; however, this change will likely break a lot of pending patches.
> 
> 
> @GregKH: Could you apply all pending patches in preparation for this patch?
> If so, then Pavel could make this transformation while the list is
> relatively idle.

Everything up to last week was already merged, and my tree is now closed
due to the merge window.  So send away.  Merge issues can be addressed
over time, and rebasing is always easier than making the patchset to
start with, so this shouldn't be that big of an issue.

thanks,

greg k-h