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
