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

[Daniil Tatianin]

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@xxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxx>
>
> Thanks,
> Michal
>
> [...]
> > --
> > 2.25.1