[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <SJ0PR18MB5216722A40A24102A158A923DB8BA@SJ0PR18MB5216.namprd18.prod.outlook.com>
Date: Thu, 7 Dec 2023 04:50:46 +0000
From: Suman Ghosh <sumang@...vell.com>
To: "dinghao.liu@....edu.cn" <dinghao.liu@....edu.cn>
CC: Ariel Elior <aelior@...vell.com>,
Manish Chopra <manishc@...vell.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
Yuval Mintz <Yuval.Mintz@...gic.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [EXT] [PATCH] qed: Fix a potential double-free in
qed_cxt_tables_alloc
>> >+++ 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 = NULL;
>> > }
>> > kfree(p_mngr->ilt_shadow);
>> >+ p_hwfn->p_cxt_mngr->ilt_shadow = NULL;
>> [Suman] Hi Dinghao,
>>
>> I am not sure how this will help prevent the double free here? If
>> qed_ilt_shadow_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
>> >
>
>kfree(NULL) is safe in kernel. But kfree() will not set the freed
>pointer to NULL. Therefore, checking p_mngr->ilt_shadow will not prevent
>the kfree() for the second time. Many double-free bugs are fixed by
>setting the freed pointer to NULL (e.g.,
>6b0d0477fce7 ("media: dvb-core: Fix double free in
>dvb_register_device()")), so I just fix this bug in the same way.
>
>Regards,
>Dinghao
[Suman] Okay, I understand. Along with this change, I have couple of suggestion. But it is up to you to make them.
1. In the beginning of the function qed_ilt_shadow_free() should we add a check and return if ilt_shadew == NULL? So, that the control does not reach to the end of the function?
2. I see in qed_ilt_shadow_alloc() we are calling "goto ilt_shadow_fail" even if the kcalloc() is failing. If kcalloc() fails, then there is nothing to free, and we can directly return from there, right?
Powered by blists - more mailing lists