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 <[email protected]>
---
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
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
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
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