Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
From:   Suman Ghosh <sumang@marvell.com>
To:     Dinghao Liu <dinghao.liu@zju.edu.cn>
CC:     Ariel Elior <aelior@marvell.com>,
        Manish Chopra <manishc@marvell.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Yuval Mintz <Yuval.Mintz@qlogic.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [EXT] [PATCH] qed: Fix a potential double-free in
 qed_cxt_tables_alloc
Thread-Topic: [EXT] [PATCH] qed: Fix a potential double-free in
 qed_cxt_tables_alloc
Thread-Index: AQHaKA/xsnR+9jaHEE6ZkoDXc2rqTrCcdTjA
Date:   Wed, 6 Dec 2023 16:43:09 +0000
Message-ID: <SJ0PR18MB5216D284A0CFEF612721D5FDDB84A@SJ0PR18MB5216.namprd18.prod.outlook.com>
References: <20231206064531.6089-1-dinghao.liu@zju.edu.cn>
In-Reply-To: <20231206064531.6089-1-dinghao.liu@zju.edu.cn>
Accept-Language: en-IN, en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?mfdSDYVEYVdpjpd1yzkMuA8SweO7a7J9Yin9ugnXK/5f15XH0DLSNrjIaAfS?=
 =?us-ascii?Q?5eTEYF7dtOk5lEgOVLUUzzsJdwSgPuFdd8HFhr3AVOCb41byqSFu9eBva+qs?=
 =?us-ascii?Q?5Bz0arDcl6gv+gTXktFePvKlPK5T0Hw9rdA0AK1Wrpb14NIseh1I9oJrN8XD?=
 =?us-ascii?Q?AWeqOyHflLbbA6hpj+CVHuTd3bHa18vTdckLTk9t8t4V0/bmzb4d53MnaLCw?=
 =?us-ascii?Q?HQ1c8WGta0uVRlJ1aeTTOdlhdxqi8ZZbAJtcKCCBg5fnzwap/ZL6dXn7xEMd?=
 =?us-ascii?Q?BlCeUVXPNpjvmP2NbgBF6H0TA28Z5RllknP4BOxSTLQ9Fxe0r288tPt+zcK8?=
 =?us-ascii?Q?d9NEDvU3t/B2/7JzEm8VZnY7X+3TlwjWfEdDjjKDF9crjgKPCdHC6K+XHmc9?=
 =?us-ascii?Q?O9uRWnSnp61bd3tr/hExnWanLLGyURjQ6FsaDVQYRWrgiOB26Guv6c7hkuBa?=
 =?us-ascii?Q?PqlF1uSby9SoeTFgUggcpj0SKc3qPGo9bUhnmfYwLSIXSHbqZuy2xNAd5Zw8?=
 =?us-ascii?Q?DJK8g3IH63SfR6D9W3f7+vGOkQbfJ7CLzd189UwkAlIInA2Zqb1CP1XoKTjP?=
 =?us-ascii?Q?DtCE4rFg16dggkuCFrjj7+AaDzLaYw72EXRZ4CfhhPXAxHuWvyDM143xfonR?=
 =?us-ascii?Q?lcg7tzCPRezQRsiWcB7336/Z4wB44zFnmj8nhG470Kmc1w5Jcxyu4xbDCF5h?=
 =?us-ascii?Q?+Y2TBYq+UC4543w2hRZSVKAFwJ2oIiAjz7il9DyV4usl3vdyGfiuFEHKdmo/?=
 =?us-ascii?Q?W3NkaQOfx3tLIC2+/H98tKEkqddZP6Dapiezl7RjBjEg3HscGCkNDYgr6fDK?=
 =?us-ascii?Q?B7EG5yoEb4Z7M2Sfgx1QdgicgQuZv/2XhHr5aks2lIQM1dvjB7gkLAXtWYnP?=
 =?us-ascii?Q?aHjYcF2zQSszegNU/ZtsXYTomxtJKp441P0ec5WrgNhBmJoaqzjnwMeyJDbv?=
 =?us-ascii?Q?hlpEGB1oclZdQwlH1uLECTh8aPDhs4EkTdH8mXFG+/+RYAYmMg6/+Ml4tOuL?=
 =?us-ascii?Q?4AOZSUg7JuFLl0Vmm1j3LlSb0iPxOQF8IC28+5x4AzkN08cxSZCT2ZzFyb2Q?=
 =?us-ascii?Q?3PmVIjBCugwJ+25DELHqGMiJtPv2CBKAyN/4jtue5g2zRMnSB+x4cAkmyAkz?=
 =?us-ascii?Q?FJ22XCH0B5XOlcvUS4qFBbGYjkUtZLcNuU1Axw62q/ZMDPmFWYU0fiXrxXoO?=
 =?us-ascii?Q?hOYmJLk1ic2fgBYNzicilihampFzlAKzSQhCYXV9N6VVAUgC66w+rq1mXm+g?=
 =?us-ascii?Q?ncyTOjdGIEm7YJe8ulKb870mnikmD9RXzrlKmrgk2kO5PfF8LCwg1+AqE+R8?=
 =?us-ascii?Q?h8M/DWD8yS0qClusvuKWqxQ8ttC6wNSUvcGFWja45j0TF6B4Na+nVxvnxMlr?=
 =?us-ascii?Q?xP7P8UaCU4XSqfA4ln/30FfwfkQ/BzLBCRxiZGFk0+EqbDrGEw4ig2eQNn09?=
 =?us-ascii?Q?Gph9Alk429RH2nRk+nSIRbntDuzOc9VX6behz2EgUanTt84wS9cy3vhjuAYG?=
 =?us-ascii?Q?b4r4aKy7nTsupUwLbUMzSBorHu6Ju2OeoRv+6H8Z9zJFhHICqbbkBLPtzuiN?=
 =?us-ascii?Q?ri8ja7Tejl4mR8Z1JQ/xykepnGhN4Ixs8aJd9QJ/?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: marvell.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SJ0PR18MB5216.namprd18.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2ebbed97-ac91-4076-3222-08dbf67a6e22
X-MS-Exchange-CrossTenant-originalarrivaltime: 06 Dec 2023 16:43:09.6969
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 70e1fb47-1155-421d-87fc-2e58f638b6e0
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: +iE/mT0TqjTBoxR95rat1C/AbmLSsQ75tyKXeVkRQ0fM45oSa+cyPOJTDNcWQaR06y26/fZvO5djvlfwtcYz/A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR18MB3885
X-Proofpoint-ORIG-GUID: LfkJiZrBJrMeHzzL-SnyZbiMWtQEGVZ1
X-Proofpoint-GUID: LfkJiZrBJrMeHzzL-SnyZbiMWtQEGVZ1
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.997,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2023-12-06_14,2023-12-06_01,2023-05-22_02
X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,
	SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Wed, 06 Dec 2023 08:43:28 -0800 (PST)

>qed_ilt_shadow_alloc() will call qed_ilt_shadow_free() to free p_hwfn-
>>p_cxt_mngr->ilt_shadow on error. However,
>qed_cxt_tables_alloc() frees this pointer again on failure of
>qed_ilt_shadow_alloc() through calling qed_cxt_mngr_free(), which may
>lead to double-free. Fix this issue by setting p_hwfn->p_cxt_mngr-
>>ilt_shadow to NULL in qed_ilt_shadow_free().
>
>Fixes: fe56b9e6a8d9 ("qed: Add module with basic common support")
>Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>
>---
> drivers/net/ethernet/qlogic/qed/qed_cxt.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/drivers/net/ethernet/qlogic/qed/qed_cxt.c
>b/drivers/net/ethernet/qlogic/qed/qed_cxt.c
>index 65e20693c549..26e247517394 100644
>--- a/drivers/net/ethernet/qlogic/qed/qed_cxt.c
>+++ b/drivers/net/ethernet/qlogic/qed/qed_cxt.c
>@@ -933,6 +933,7 @@ static void qed_ilt_shadow_free(struct qed_hwfn
>*p_hwfn)
> 		p_dma->virt_addr =3D NULL;
> 	}
> 	kfree(p_mngr->ilt_shadow);
>+	p_hwfn->p_cxt_mngr->ilt_shadow =3D NULL;
[Suman] Hi Dinghao,

I am not sure how this will help prevent the double free here? If qed_ilt_s=
hadow_alloc() fails to allocate memory, then still qed_cxt_mngr_free() will=
 be called and kfree()
will try to free the NULL pointer. Shouldn't it be like below?

if (p_mngr->ilt_shadow)
	Kfree(p_mngr->ilt_shadow);
> }
>
> static int qed_ilt_blk_alloc(struct qed_hwfn *p_hwfn,
>--
>2.17.1
>