Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sat, 11 Sep 2021 10:30:56 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Saurav Girepunje <saurav.girepunje@gmail.com>
Cc:     saurav.girepunje@hotmail.com, Larry.Finger@lwfinger.net,
        florian.c.schilhabel@googlemail.com, gregkh@linuxfoundation.org,
        fmdefrancesco@gmail.com, linux-staging@lists.linux.dev,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] staging: rtl8712: Move similar execution in to a
 function.
Message-ID: <20210911073056.GE7203@kadam>
References: <YTsXXxtQn7QN6nIm@user>
 <20210910090045.GD7203@kadam>
 <a31836c3-6999-339b-32f8-5f7d7f7e5b27@gmail.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <a31836c3-6999-339b-32f8-5f7d7f7e5b27@gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?tnox9dLUTKwgGRHuCnauqE8JVY+SL6nDUCwCTiQRcNErLg66nAfAIjsuyCo7?=
 =?us-ascii?Q?qk5JMR1y0uUGQwWZPdhzhuIfu7rHVLVrths7jtvSlCH58wA5K0Sdq2yin/Uy?=
 =?us-ascii?Q?BC7y54FJsNrTuzQ2iDo/NAKM7Y7+kL8QZSadWoPi9FEnYBEgsTEwu+XLMbv4?=
 =?us-ascii?Q?LHO4DibyxciS/jDFFsbqmMcWONwbEVxoQSrfx+A4nwUDBU7F+y24M69Tp8zr?=
 =?us-ascii?Q?3aeRD5gLWxu6YcLgUQc6cf+Qhk15Xl663njWxXprkf2hN+wGwiHffVyEg5hm?=
 =?us-ascii?Q?Xna154tyJaur1Lo7MzJRS6nkSnECRoOaTCg6VH5JxVE1vJBL9qaYkT+D4XuS?=
 =?us-ascii?Q?ecvq10nFzd7q5yS8VMzZq4sqNeh7sZguYuUmw7ThbmiscBn2P1Eo5Lu0ixGn?=
 =?us-ascii?Q?d/henfDmZvyEO7a9Z5zF42fhJnS8qyZlQeZFxeDpkK6SuGKC7RuibTzEnzyh?=
 =?us-ascii?Q?MO+D/sPLxmeahn+31qVSpvIwCH70z11U4ZC004zUyRXReuAVN+fMUL8vY7/K?=
 =?us-ascii?Q?vvi3LNHVM1pq3bB1fe9hG/PiwbcarFWweXdesLsHfByQyI6gijU7bt9wUP7L?=
 =?us-ascii?Q?XIoUEJ054nqg5GotDJs+mnJW0JxRW9KqfUyvcx/kUVRugZWYoolREJC9eBWE?=
 =?us-ascii?Q?3TyR0dli1lrgwatUSmrmgPW9DbAZPNaMXhwhpRG20sQXJ3MS60iks2r3kk6H?=
 =?us-ascii?Q?Ovt22UwJieB6+12H5z1Zhht/pLvs9BBltfsN/+scDX4Tqe5FZ9ZMRXgfaUjt?=
 =?us-ascii?Q?yqE0IWdyaQROyst5D8lxwLD2xKsM7AKdobpcAhDPfSYIIVptkOuey7t2v+gh?=
 =?us-ascii?Q?+dT5cLJFBSxI0kKK6CBwI97GEHyZtcI9i/VHotpLFEltGyxCY4GzIYBffQeN?=
 =?us-ascii?Q?4lFx94CyTG5PpNPpNJ00m7JIIHjElEtxChxh7cY2jh6K1J2wv1G62s+0m9/Q?=
 =?us-ascii?Q?nhy/s74h22cmoYV2nevRTkhCIaF8BXDp7aTBr0om53luFvG/9c0j287vWewQ?=
 =?us-ascii?Q?OkN1xzopMpXnwDDJx5WRDA7OEQ343umfkDfxO5CKJwoniHSintqDT9kCFr9c?=
 =?us-ascii?Q?TuH1Fk69ycfzmi9O09ioxpfM9EEmDb2y/vDNHpdPEMmn+/XXKbveusHfyk/b?=
 =?us-ascii?Q?BDbD9P/+3rK9tSE7i11sNxzjNsHj5ZLjSWsb8qdS5z3bOzPVM0FihXn98//v?=
 =?us-ascii?Q?1M/uHht4FM/NVCyvtsaQH/HAwyAvpdVnz7BKPqgwVdF4Cj2dxwSfF7dy7mPL?=
 =?us-ascii?Q?MwFkFspOc5fF/vlu9v41TKTlv103uEdCRMyQ2ceLZEqKhew1yC5VyM8YR9A1?=
 =?us-ascii?Q?0yaF7x5yqjYluhnk1ciLWoaw?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a466249f-dd48-4ce4-2ef7-08d974f621d0
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Sep 2021 07:31:13.3520
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: JapAAA6VuBWV9XQrDXgJUJ9cHnZnMioWjhQyBO7PDaAckSYQyUkPExHxWMlrstgMCQf8ToiypzPp2HlcU6i+chmPdVKUDqLGcR50tyqRDr8=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR10MB1950
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10103 signatures=668682
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 mlxscore=0
 mlxlogscore=999 adultscore=0 bulkscore=0 spamscore=0 phishscore=0
 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2109030001 definitions=main-2109110045
X-Proofpoint-GUID: 0Tw6gExSZjcKNETO54kOaAkPVNbLbEqv
X-Proofpoint-ORIG-GUID: 0Tw6gExSZjcKNETO54kOaAkPVNbLbEqv
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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