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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Sat, 11 Sep 2021 10:30:56 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Saurav Girepunje <saurav.girepunje@...il.com>
Cc:     saurav.girepunje@...mail.com, 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
Subject: Re: [PATCH v2] staging: rtl8712: Move similar execution in to a
 function.

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