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:   Fri, 10 Sep 2021 12:00:45 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Saurav Girepunje <saurav.girepunje@gmail.com>
Cc:     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,
        saurav.girepunje@hotmail.com
Subject: Re: [PATCH v2] staging: rtl8712: Move similar execution in to a
 function.
Message-ID: <20210910090045.GD7203@kadam>
References: <YTsXXxtQn7QN6nIm@user>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YTsXXxtQn7QN6nIm@user>
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?lU97j+7d6TRo5Yq1fWgOZVza/+ZIGInpJNZJrKXP+Anmw4kbF/H4wlAOpT6i?=
 =?us-ascii?Q?/kMKs1OshxVuKYKoBhehHKxuXrDtscJho6SbYISMEKtF3bMOa+jzdtZ2QvJy?=
 =?us-ascii?Q?1sTT8KzFmJe2WRa2UhkTHnMDc5bI8ukQwglkQ7/LvoKYYrbY/ZPT2WZYp4RC?=
 =?us-ascii?Q?jL4iAd42BnO2gPMOqt5E9Ezs6TFff2EqjhcijtOS1xtopdaeJEuoVxctz/9h?=
 =?us-ascii?Q?S1NdO/yPYB+1yHobKMwkJNz0L7ANetG0tWL5yNWLgOFUb/j8NCG8dG4M7Yoi?=
 =?us-ascii?Q?dbpnDTJlBlUUK4cx4N9E/MbsgR69jQoyUtDZh5K56m6RapOovlxbb/pdEVLW?=
 =?us-ascii?Q?QyNv7jmA4CwMoYN4gXrUQKqhOaDWkwHEX7ARTaNM/lCQ+6bReMx4ZPnIW0Ck?=
 =?us-ascii?Q?sT+7ZBhJbzi+QPz8mR6vN2tDO/9Grz6PGfewBlK0XL2XaAdpC0Z8op2vMEPw?=
 =?us-ascii?Q?l61JXUpBOL1FQ6KVVkv8EGbKz51Pmj6hREVtAOtHUfhVjRjJQpxjNlYOfBob?=
 =?us-ascii?Q?V7KYzYWYwhSgV2Jp4CrLp0MxnDRIuI/ZmGVpZCEZ6TViVsGb3VMRnO7A/yGp?=
 =?us-ascii?Q?Opvd4KInKo6bRUSV2bN3PZFwSB7U8PBbEboBPaaSQOHLSlYvG+S9kygTr2N0?=
 =?us-ascii?Q?DpPfgXzeVPOkTpP7lzirzeIZvAqpJkl7GMeEHcPjbtBs1F8YVV94Ro2Er3OH?=
 =?us-ascii?Q?iHZ4fgQfF/vKd+mEwn4esyqDMPh4gX3ju9ZghtKIT/xGuFIIQt7HYCvS9oQ4?=
 =?us-ascii?Q?b8lz13d8bjfcICj/3VCHK7m9ILyYk7eFIkJwlNh/uSbuD3tJOgQpNdLBQJli?=
 =?us-ascii?Q?qqeABOaXuJDopkatr2FhpQwBbtD6DqmOWVC+KbMjXOkaGWbfFMIzRkzCCe4A?=
 =?us-ascii?Q?SpREqYzmcqx+A0qdWTpCMQz2i0i3bNoQVsEWFUU02V92gZ3oJnzo1clVlYey?=
 =?us-ascii?Q?iNfdkVFdDYoL8l3umeEfml4o8AiSZYetSS5QtLbLed5MlHpAvvFrDs2IueKm?=
 =?us-ascii?Q?IxfuzdVSPllVtn3YB6atVjF2Oxi1fwBz2+oUg99MYpgxOs4IBcXcgKVEg3as?=
 =?us-ascii?Q?VeqsKcBenwFvqtl4E7n1ovMMNqAXMIlfWp6ZLSQgO7Dsb+GVCRPtAmxJ+4Hj?=
 =?us-ascii?Q?45l6WiGu4jqRlbian77d05dvMDtB5VCyDKTD4uptQR9+FVrv8Cm7+PjOLOh+?=
 =?us-ascii?Q?0uji8sU+O8TLrfA0qi5JNWp+JxXtAiSlKgChkKmkrk5IteHtvl0jIBKPrPuX?=
 =?us-ascii?Q?Kbtuz5PhssQUO1s4E5WJ0dFRD9S9BVNvZSSajT65XV3xv1wY9wY8mMq+wPmD?=
 =?us-ascii?Q?I2iXExSuTmY/0FNXZbQ4Yiwi?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: dcc8558e-0c2d-4add-0bf3-08d974398410
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Sep 2021 09:01:03.1315
 (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: cbP5JCCI7wgvCVgWiwqyGM37/jtrLwadCvTRaDAKOipaoU61sQQrfUaCagg6+vXdX7q2bKZSGLXSpEhy4xiTGzi4ZJCtc8N5exmeNjh4FfQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR10MB4449
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10102 signatures=668682
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 bulkscore=0 phishscore=0
 spamscore=0 malwarescore=0 suspectscore=0 mlxscore=0 mlxlogscore=999
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2109030001
 definitions=main-2109100058
X-Proofpoint-GUID: AarM69_CcaSrfRK1D8h3CM-_NQE2xyrc
X-Proofpoint-ORIG-GUID: AarM69_CcaSrfRK1D8h3CM-_NQE2xyrc
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


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