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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 22 Dec 2022 14:46:32 +0300 From: Daniil Tatianin <d-tatianin@...dex-team.ru> To: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com> Cc: Shahed Shaikh <shshaikh@...vell.com>, Manish Chopra <manishc@...vell.com>, GR-Linux-NIC-Dev@...vell.com, "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH net v2] qlcnic: prevent ->dcb use-after-free on qlcnic_dcb_enable() failure On 12/22/22 12:57 PM, Michal Swiatkowski wrote: > On Thu, Dec 22, 2022 at 10:42:23AM +0300, Daniil Tatianin wrote: >> adapter->dcb would get silently freed inside qlcnic_dcb_enable() in >> case qlcnic_dcb_attach() would return an error, which always happens >> under OOM conditions. This would lead to use-after-free because both >> of the existing callers invoke qlcnic_dcb_get_info() on the obtained >> pointer, which is potentially freed at that point. >> >> Propagate errors from qlcnic_dcb_enable(), and instead free the dcb >> pointer at callsite using qlcnic_dcb_free(). This also removes the now >> unused qlcnic_clear_dcb_ops() helper, which was a simple wrapper around >> kfree() also causing memory leaks for partially initialized dcb. >> >> Found by Linux Verification Center (linuxtesting.org) with the SVACE >> static analysis tool. >> >> Fixes: 3c44bba1d270 ("qlcnic: Disable DCB operations from SR-IOV VFs") >> Signed-off-by: Daniil Tatianin <d-tatianin@...dex-team.ru> >> --- >> Changes since v1: >> - Add a fixes tag + net as a target >> - Remove qlcnic_clear_dcb_ops entirely & use qlcnic_dcb_free instead >> --- >> drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_init.c | 9 ++++++++- >> drivers/net/ethernet/qlogic/qlcnic/qlcnic_dcb.h | 10 ++-------- >> drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c | 9 ++++++++- >> 3 files changed, 18 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_init.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_init.c >> index dbb800769cb6..774f2c6875ec 100644 >> --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_init.c >> +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_init.c >> @@ -2505,7 +2505,14 @@ int qlcnic_83xx_init(struct qlcnic_adapter *adapter) >> goto disable_mbx_intr; >> >> qlcnic_83xx_clear_function_resources(adapter); >> - qlcnic_dcb_enable(adapter->dcb); >> + >> + err = qlcnic_dcb_enable(adapter->dcb); >> + if (err) { >> + qlcnic_dcb_free(adapter->dcb); >> + adapter->dcb = NULL; > Small nit, I think qlcnic_dcb_free() already set adapter->dcb to NULL. Oops, you're right. Thanks for spotting. > Otherwise, thanks for changing: > Reviewed-by: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com> > > Thanks, > Michal > > [...] >> -- >> 2.25.1
Powered by blists - more mailing lists