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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210910090045.GD7203@kadam>
Date:   Fri, 10 Sep 2021 12:00:45 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Saurav Girepunje <saurav.girepunje@...il.com>
Cc:     Larry.Finger@...inger.net, florian.c.schilhabel@...glemail.com,
        gregkh@...uxfoundation.org, fmdefrancesco@...il.com,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org,
        saurav.girepunje@...mail.com
Subject: Re: [PATCH v2] staging: rtl8712: Move similar execution in to a
 function.


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