Messages in this thread | | | Date | Wed, 21 Dec 2022 10:47:29 +0300 | Subject | Re: [PATCH v1] qlcnic: prevent ->dcb use-after-free on qlcnic_dcb_enable() failure | From | Daniil Tatianin <> |
| |
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@yandex-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 >>
| |