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

Message-ID: <13642c61.22e78.18c4306e6f2.Coremail.dinghao.liu@zju.edu.cn>
Date: Thu, 7 Dec 2023 14:46:15 +0800 (GMT+08:00)
From: dinghao.liu@....edu.cn
To: "Suman Ghosh" <sumang@...vell.com>
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?

Thanks for your suggestions! For the first suggestion, ilt_shadew is
checked in the beginning of the for loop, but it seems that we need
not to check this pointer at each iteration. I will move it to the
beginning of the function. I also rechecked the loop and found the bug
here should be a use-after-free instead of double-free. There is a 
pointer dereference &p_mngr->ilt_shadow[i], which will be
triggered before kfree(). I will resend a new patch to fix this.

For the second suggestion, I agree that directly returning would be better.
I will fix this in the next patch version, thanks!

Regards,
Dinghao