linux-kernel - Re: [PATCH 4.19 47/48] i40e: Memory leak in i40e_config_iwarp

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 linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 11 Aug 2020 09:45:04 -0700
From:   Jesse Brandeburg <jesse.brandeburg@...el.com>
To:     Pavel Machek <pavel@...x.de>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        <linux-kernel@...r.kernel.org>, <stable@...r.kernel.org>,
        Martyna Szapar <martyna.szapar@...el.com>,
        Andrew Bowers <andrewx.bowers@...el.com>,
        "Jeff Kirsher" <jeffrey.t.kirsher@...el.com>
Subject: Re: [PATCH 4.19 47/48] i40e: Memory leak in
 i40e_config_iwarp_qvlist

On Tue, 11 Aug 2020 14:46:14 +0200
Pavel Machek <pavel@...x.de> wrote:

> > --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> > @@ -449,16 +450,19 @@ static int i40e_config_iwarp_qvlist(stru
> >  			 "Incorrect number of iwarp vectors %u.
> > Maximum %u allowed.\n", qvlist_info->num_vectors,
> >  			 msix_vf);
> > -		goto err;
> > +		ret = -EINVAL;
> > +		goto err_out;
> >  	}
> 
> And it is no longer freeing data qvlist_info() in this path. Is that
> correct? Should it goto err_free instead? 

Hi Pavel, thanks for the review.

I believe it is still correct, the logic is a bit convoluted, but
tracing back, I see that the caller in i40e_main.c allocates a buffer,
calls this function (eventually) with that memory cast to the *input*
variable qvlist_info, and then the top caller frees the original buffer.

One thing that I'll admit is confusing here is that the *input* struct
qvlist_info is different than the vf->qvlist_info struct managed by
this function. Maybe that they have the same name was confusing to you?
It confused me for a moment while I investigated, but I believe there
is no actual problem.

The reason for the function working this way is that the input data is
from the VF message, and the driver data structures in the PF (i40e)
driver representing state of the VF are managed separately. 

Jesse