[PATCH v2] staging: rtl8712: Move similar execution in to a function.

[PATCH v2] staging: rtl8712: Move similar execution in to a function.

[Saurav Girepunje]

In rtl8712_cmd.c function read_macreg_hdl,write_macreg_hdl,write_bbreg_hdl
and write_rfreg_hdl all are having same execution.

Move this common execution in to a new function common_read_write_hdl().
Call common_read_write_hdl() from read_macreg_hdl,write_macreg_hdl,
write_bbreg_hdlhis and write_rfreg_hdl.This will reduce duplicate code.

Signed-off-by: Saurav Girepunje <saurav.girepunje@gmail.com>
---

ChangeLog V2:
- Add more explanation about the patch

 drivers/staging/rtl8712/rtl8712_cmd.c | 41 +++++++--------------------
 1 file changed, 11 insertions(+), 30 deletions(-)

diff --git a/drivers/staging/rtl8712/rtl8712_cmd.c b/drivers/staging/rtl8712/rtl8712_cmd.c
index e9294e1ed06e..9bc0588be04b 100644
--- a/drivers/staging/rtl8712/rtl8712_cmd.c
+++ b/drivers/staging/rtl8712/rtl8712_cmd.c
@@ -117,9 +117,9 @@ static void r871x_internal_cmd_hdl(struct _adapter *padapter, u8 *pbuf)
 	kfree(pdrvcmd->pbuf);
 }

-static u8 read_macreg_hdl(struct _adapter *padapter, u8 *pbuf)
+static u8 common_read_write_hdl(struct _adapter *padapter, u8 *pbuf)
 {
-	void (*pcmd_callback)(struct _adapter *dev, struct cmd_obj	*pcmd);
+	void (*pcmd_callback)(struct _adapter *dev, struct cmd_obj *pcmd);
 	struct cmd_obj *pcmd  = (struct cmd_obj *)pbuf;

 	/*  invoke cmd->callback function */
@@ -129,20 +129,17 @@ static u8 read_macreg_hdl(struct _adapter *padapter, u8 *pbuf)
 	else
 		pcmd_callback(padapter, pcmd);
 	return H2C_SUCCESS;
+
 }

-static u8 write_macreg_hdl(struct _adapter *padapter, u8 *pbuf)
+static u8 read_macreg_hdl(struct _adapter *padapter, u8 *pbuf)
 {
-	void (*pcmd_callback)(struct _adapter *dev, struct cmd_obj	*pcmd);
-	struct cmd_obj *pcmd  = (struct cmd_obj *)pbuf;
+	return common_read_write_hdl(padapter, pbuf);
+}

-	/*  invoke cmd->callback function */
-	pcmd_callback = cmd_callback[pcmd->cmdcode].callback;
-	if (!pcmd_callback)
-		r8712_free_cmd_obj(pcmd);
-	else
-		pcmd_callback(padapter, pcmd);
-	return H2C_SUCCESS;
+static u8 write_macreg_hdl(struct _adapter *padapter, u8 *pbuf)
+{
+	return common_read_write_hdl(padapter, pbuf);
 }

 static u8 read_bbreg_hdl(struct _adapter *padapter, u8 *pbuf)
@@ -155,15 +152,7 @@ static u8 read_bbreg_hdl(struct _adapter *padapter, u8 *pbuf)

 static u8 write_bbreg_hdl(struct _adapter *padapter, u8 *pbuf)
 {
-	void (*pcmd_callback)(struct _adapter *dev, struct cmd_obj *pcmd);
-	struct cmd_obj *pcmd  = (struct cmd_obj *)pbuf;
-
-	pcmd_callback = cmd_callback[pcmd->cmdcode].callback;
-	if (!pcmd_callback)
-		r8712_free_cmd_obj(pcmd);
-	else
-		pcmd_callback(padapter, pcmd);
-	return H2C_SUCCESS;
+	return common_read_write_hdl(padapter, pbuf);
 }

 static u8 read_rfreg_hdl(struct _adapter *padapter, u8 *pbuf)
@@ -184,15 +173,7 @@ static u8 read_rfreg_hdl(struct _adapter *padapter, u8 *pbuf)

 static u8 write_rfreg_hdl(struct _adapter *padapter, u8 *pbuf)
 {
-	void (*pcmd_callback)(struct _adapter *dev, struct cmd_obj *pcmd);
-	struct cmd_obj *pcmd  = (struct cmd_obj *)pbuf;
-
-	pcmd_callback = cmd_callback[pcmd->cmdcode].callback;
-	if (!pcmd_callback)
-		r8712_free_cmd_obj(pcmd);
-	else
-		pcmd_callback(padapter, pcmd);
-	return H2C_SUCCESS;
+	return common_read_write_hdl(padapter, pbuf);
 }

 static u8 sys_suspend_hdl(struct _adapter *padapter, u8 *pbuf)
--
2.32.0

Re: [PATCH v2] staging: rtl8712: Move similar execution in to a function.

[Dan Carpenter]

On Fri, Sep 10, 2021 at 01:59:19PM +0530, Saurav Girepunje wrote:
> In rtl8712_cmd.c function read_macreg_hdl,write_macreg_hdl,write_bbreg_hdl
> and write_rfreg_hdl all are having same execution.

I get what you're trying to do, because this code is bad and duplicative
but this is not the right fix.

Let's take read_macreg_hdl() as an example.

Look at how it's called:

   215          switch (pcmd->cmdcode) {
   216          case GEN_CMD_CODE(_Read_MACREG):
   217                  read_macreg_hdl(padapter, (u8 *)pcmd);
   218                  pcmd_r = pcmd;
   219                  break;

Then look at how it's implemented:

   120  static u8 read_macreg_hdl(struct _adapter *padapter, u8 *pbuf)
   121  {
   122          void (*pcmd_callback)(struct _adapter *dev, struct cmd_obj      *pcmd);
   123          struct cmd_obj *pcmd  = (struct cmd_obj *)pbuf;
   124  
   125          /*  invoke cmd->callback function */
   126          pcmd_callback = cmd_callback[pcmd->cmdcode].callback;

So pcmd->cmdcode is GEN_CMD_CODE(_Read_MACREG).  We look that up in the
cmd_callback[] array and it is:

        {GEN_CMD_CODE(_Read_MACREG), NULL}, /*0*/

   127          if (!pcmd_callback)
   128                  r8712_free_cmd_obj(pcmd);

So now we no that "pcmd_callback" is NULL meaning it will free "pcmd".
And if you remember in the caller it does "pcmd_r = pcmd;" but "pcmd"
is freed so that's going to lead to a use after free in r8712_cmd_thread().
It's garbage and the patch doesn't really help.

The right way to fix it is to get rid of the cmd_callback[] array.

   129          else
   130                  pcmd_callback(padapter, pcmd);
   131          return H2C_SUCCESS;
   132  }

Getting rid of the cmd_callback[] array looks like this.  In
read_rfreg_hdl() we know that the callback is r8712_getbbrfreg_cmdrsp_callback()
so we can call that directly.

diff --git a/drivers/staging/rtl8712/rtl8712_cmd.c b/drivers/staging/rtl8712/rtl8712_cmd.c
index e9294e1ed06e..51a6abb27d41 100644
--- a/drivers/staging/rtl8712/rtl8712_cmd.c
+++ b/drivers/staging/rtl8712/rtl8712_cmd.c
@@ -174,11 +174,7 @@ static u8 read_rfreg_hdl(struct _adapter *padapter, u8 *pbuf)
 
 	if (pcmd->rsp && pcmd->rspsz > 0)
 		memcpy(pcmd->rsp, (u8 *)&val, pcmd->rspsz);
-	pcmd_callback = cmd_callback[pcmd->cmdcode].callback;
-	if (!pcmd_callback)
-		r8712_free_cmd_obj(pcmd);
-	else
-		pcmd_callback(padapter, pcmd);
+	r8712_getbbrfreg_cmdrsp_callback(padapter, pcmd);
 	return H2C_SUCCESS;
 }
 

regards,
dan carpenter

Re: [PATCH v2] staging: rtl8712: Move similar execution in to a function.

[Saurav Girepunje]

On 10/09/21 2:30 pm, Dan Carpenter wrote:
> 
> On Fri, Sep 10, 2021 at 01:59:19PM +0530, Saurav Girepunje wrote:
>> In rtl8712_cmd.c function read_macreg_hdl,write_macreg_hdl,write_bbreg_hdl
>> and write_rfreg_hdl all are having same execution.
> 
> I get what you're trying to do, because this code is bad and duplicative
> but this is not the right fix.
> 
> Let's take read_macreg_hdl() as an example.
> 
> Look at how it's called:
> 
>     215          switch (pcmd->cmdcode) {
>     216          case GEN_CMD_CODE(_Read_MACREG):
>     217                  read_macreg_hdl(padapter, (u8 *)pcmd);
>     218                  pcmd_r = pcmd;
>     219                  break;
> 
> Then look at how it's implemented:
> 
>     120  static u8 read_macreg_hdl(struct _adapter *padapter, u8 *pbuf)
>     121  {
>     122          void (*pcmd_callback)(struct _adapter *dev, struct cmd_obj      *pcmd);
>     123          struct cmd_obj *pcmd  = (struct cmd_obj *)pbuf;
>     124
>     125          /*  invoke cmd->callback function */
>     126          pcmd_callback = cmd_callback[pcmd->cmdcode].callback;
> 
> So pcmd->cmdcode is GEN_CMD_CODE(_Read_MACREG).  We look that up in the
> cmd_callback[] array and it is:
> 
>          {GEN_CMD_CODE(_Read_MACREG), NULL}, /*0*/
> 
>     127          if (!pcmd_callback)
>     128                  r8712_free_cmd_obj(pcmd);
> 
> So now we no that "pcmd_callback" is NULL meaning it will free "pcmd".
> And if you remember in the caller it does "pcmd_r = pcmd;" but "pcmd"
> is freed so that's going to lead to a use after free in r8712_cmd_thread().
> It's garbage and the patch doesn't really help.

One more thought here after the

     127          if (!pcmd_callback)
     128                  r8712_free_cmd_obj(pcmd);

r8712_free_cmd_obj(pcmd); we could do  pcmd = NULL; so in the caller 
when it will do "pcmd_r = pcmd;" it is actually making NULL to pcmd_r. 
On r8712_cmd_thread there is check for pcmd is NULL or not before 
execution on pcmd.

pcmd = cmd_hdl_filter(padapter, pcmd);
	if (pcmd) { /* if pcmd != NULL, cmd will be handled by f/w */

Please let me know you thought on this dan.

> 
> The right way to fix it is to get rid of the cmd_callback[] array.
> 
>     129          else
>     130                  pcmd_callback(padapter, pcmd);
>     131          return H2C_SUCCESS;
>     132  }
> 
> Getting rid of the cmd_callback[] array looks like this.  In
> read_rfreg_hdl() we know that the callback is r8712_getbbrfreg_cmdrsp_callback()
> so we can call that directly.
> 
> diff --git a/drivers/staging/rtl8712/rtl8712_cmd.c b/drivers/staging/rtl8712/rtl8712_cmd.c
> index e9294e1ed06e..51a6abb27d41 100644
> --- a/drivers/staging/rtl8712/rtl8712_cmd.c
> +++ b/drivers/staging/rtl8712/rtl8712_cmd.c
> @@ -174,11 +174,7 @@ static u8 read_rfreg_hdl(struct _adapter *padapter, u8 *pbuf)
>   
>   	if (pcmd->rsp && pcmd->rspsz > 0)
>   		memcpy(pcmd->rsp, (u8 *)&val, pcmd->rspsz);
> -	pcmd_callback = cmd_callback[pcmd->cmdcode].callback;
> -	if (!pcmd_callback)
> -		r8712_free_cmd_obj(pcmd);
> -	else
> -		pcmd_callback(padapter, pcmd);
> +	r8712_getbbrfreg_cmdrsp_callback(padapter, pcmd);
>   	return H2C_SUCCESS;
>   }
>   
> 
> regards,
> dan carpenter
> 

Thanks Dan for review.

Regards,
Saurav Girepunje

Re: [PATCH v2] staging: rtl8712: Move similar execution in to a function.

[Dan Carpenter]

On Fri, Sep 10, 2021 at 10:19:03PM +0530, Saurav Girepunje wrote:
> 
> 
> On 10/09/21 2:30 pm, Dan Carpenter wrote:
> > 
> > On Fri, Sep 10, 2021 at 01:59:19PM +0530, Saurav Girepunje wrote:
> > > In rtl8712_cmd.c function read_macreg_hdl,write_macreg_hdl,write_bbreg_hdl
> > > and write_rfreg_hdl all are having same execution.
> > 
> > I get what you're trying to do, because this code is bad and duplicative
> > but this is not the right fix.
> > 
> > Let's take read_macreg_hdl() as an example.
> > 
> > Look at how it's called:
> > 
> >     215          switch (pcmd->cmdcode) {
> >     216          case GEN_CMD_CODE(_Read_MACREG):
> >     217                  read_macreg_hdl(padapter, (u8 *)pcmd);
> >     218                  pcmd_r = pcmd;
> >     219                  break;
> > 
> > Then look at how it's implemented:
> > 
> >     120  static u8 read_macreg_hdl(struct _adapter *padapter, u8 *pbuf)
> >     121  {
> >     122          void (*pcmd_callback)(struct _adapter *dev, struct cmd_obj      *pcmd);
> >     123          struct cmd_obj *pcmd  = (struct cmd_obj *)pbuf;
> >     124
> >     125          /*  invoke cmd->callback function */
> >     126          pcmd_callback = cmd_callback[pcmd->cmdcode].callback;
> > 
> > So pcmd->cmdcode is GEN_CMD_CODE(_Read_MACREG).  We look that up in the
> > cmd_callback[] array and it is:
> > 
> >          {GEN_CMD_CODE(_Read_MACREG), NULL}, /*0*/
> > 
> >     127          if (!pcmd_callback)
> >     128                  r8712_free_cmd_obj(pcmd);
> > 
> > So now we no that "pcmd_callback" is NULL meaning it will free "pcmd".
> > And if you remember in the caller it does "pcmd_r = pcmd;" but "pcmd"
> > is freed so that's going to lead to a use after free in r8712_cmd_thread().
> > It's garbage and the patch doesn't really help.
> 
> One more thought here after the
> 
>     127          if (!pcmd_callback)
>     128                  r8712_free_cmd_obj(pcmd);
> 
> r8712_free_cmd_obj(pcmd); we could do  pcmd = NULL; so in the caller when it
> will do "pcmd_r = pcmd;" it is actually making NULL to pcmd_r. On
> r8712_cmd_thread there is check for pcmd is NULL or not before execution on
> pcmd.
> 
> pcmd = cmd_hdl_filter(padapter, pcmd);
> 	if (pcmd) { /* if pcmd != NULL, cmd will be handled by f/w */
> 
> Please let me know you thought on this dan.

You have to look at how it's allocated and is this even called?  I
haven't looked so I don't know the answers.

regards,
dan carpenter