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: Wed, 21 Dec 2022 10:47:29 +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 v1] qlcnic: prevent ->dcb use-after-free on qlcnic_dcb_enable() failure On 12/20/22 4:32 PM, Michal Swiatkowski wrote: > On Tue, Dec 20, 2022 at 03:56:49PM +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. >> >> Found by Linux Verification Center (linuxtesting.org) with the SVACE >> static analysis tool. >> >> Signed-off-by: Daniil Tatianin <d-tatianin@...dex-team.ru> > > Please add Fix tag and net as target (net-next is close till the end of > this year) > Sorry, I'll include a fixes tag. Could you please explain what I would have to do to add net as target? >> --- >> drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_init.c | 9 ++++++++- >> drivers/net/ethernet/qlogic/qlcnic/qlcnic_dcb.h | 5 ++--- >> drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c | 9 ++++++++- >> 3 files changed, 18 insertions(+), 5 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..465f149d94d4 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_clear_dcb_ops(adapter->dcb); >> + adapter->dcb = NULL; >> + goto disable_mbx_intr; >> + } > > Maybe I miss sth but it looks like there can be memory leak. > For example if error in attach happen after allocating of dcb->cfg. > Isn't it better to call qlcnic_dcb_free instead of qlcnic_clear_dcb_ops? I think you're right, if attach fails midway then we might leak cfg or something else. I'll use qlcnic_dcb_free() instead and completely remove qlcnic_clear_dcb_ops() as it seems to be unused and relies on dcb being in a very specific state. >> + >> qlcnic_83xx_initialize_nic(adapter, 1); >> qlcnic_dcb_get_info(adapter->dcb); >> >> diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_dcb.h b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_dcb.h >> index 7519773eaca6..e1460f9c38bf 100644 >> --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_dcb.h >> +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_dcb.h >> @@ -112,9 +112,8 @@ static inline void qlcnic_dcb_init_dcbnl_ops(struct qlcnic_dcb *dcb) >> dcb->ops->init_dcbnl_ops(dcb); >> } >> >> -static inline void qlcnic_dcb_enable(struct qlcnic_dcb *dcb) >> +static inline int qlcnic_dcb_enable(struct qlcnic_dcb *dcb) >> { >> - if (dcb && qlcnic_dcb_attach(dcb)) >> - qlcnic_clear_dcb_ops(dcb); >> + return dcb ? qlcnic_dcb_attach(dcb) : 0; >> } >> #endif >> diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c >> index 28476b982bab..36ba15fc9776 100644 >> --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c >> +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c >> @@ -2599,7 +2599,14 @@ qlcnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent) >> "Device does not support MSI interrupts\n"); >> >> if (qlcnic_82xx_check(adapter)) { >> - qlcnic_dcb_enable(adapter->dcb); >> + err = qlcnic_dcb_enable(adapter->dcb); >> + if (err) { >> + qlcnic_clear_dcb_ops(adapter->dcb); >> + adapter->dcb = NULL; >> + dev_err(&pdev->dev, "Failed to enable DCB\n"); >> + goto err_out_free_hw; >> + } >> + >> qlcnic_dcb_get_info(adapter->dcb); >> err = qlcnic_setup_intr(adapter); >> >> -- >> 2.25.1 >>
Powered by blists - more mailing lists