netdev - RE: [EXT] [PATCH] qed: Fix a potential double-free in qed_cxt_tables

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] [thread-next>] [day] [month] [year] [list]

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?