Re: [PATCH v1] qlcnic: prevent ->dcb use-after-free on qlcnic_dcb_enable() failure

[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@xxxxxxxxxxxxxx>
>
> 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