linux-kernel - Re: [PATCH v2 1/6] staging: r8188eu: remove unused member free_bss

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20220417201415.GA233554@jaehee-ThinkPad-X1-Extreme>
Date:   Sun, 17 Apr 2022 16:14:15 -0400
From:   Jaehee Park <jhpark1013@...il.com>
To:     "Fabio M. De Francesco" <fmdefrancesco@...il.com>
Cc:     Larry.Finger@...inger.net, phil@...lpotter.co.uk,
        gregkh@...uxfoundation.org, linux-staging@...ts.linux.dev,
        linux-kernel@...r.kernel.org, outreachy@...ts.linux.dev,
        Pavel Skripkin <paskripkin@...il.com>
Subject: Re: [PATCH v2 1/6] staging: r8188eu: remove unused member
 free_bss_buf

On Fri, Apr 15, 2022 at 06:29:15AM +0200, Fabio M. De Francesco wrote:
> On venerdì 15 aprile 2022 04:48:32 CEST Jaehee Park wrote:
> > The free_bss_buf member of pmlmepriv is unused. Remove all related
> > lines.
> > 
> > Suggested-by: Pavel Skripkin <paskripkin@...il.com>
> > Signed-off-by: Jaehee Park <jhpark1013@...il.com>
> > ---
> >  drivers/staging/r8188eu/include/rtw_mlme.h | 1 -
> >  drivers/staging/r8188eu/core/rtw_mlme.c    | 7 -------
> >  2 files changed, 8 deletions(-)
> > 
> > diff --git a/drivers/staging/r8188eu/include/rtw_mlme.h b/drivers/
> staging/r8188eu/include/rtw_mlme.h
> > index 1dc1fbf049af..0f03ac43079c 100644
> > --- a/drivers/staging/r8188eu/include/rtw_mlme.h
> > +++ b/drivers/staging/r8188eu/include/rtw_mlme.h
> > @@ -319,7 +319,6 @@ struct mlme_priv {
> >  	struct list_head *pscanned;
> >  	struct __queue free_bss_pool;
> >  	struct __queue scanned_queue;
> > -	u8 *free_bss_buf;
> >  	u8	key_mask; /* use to restore wep key after hal_init */
> >  	u32	num_of_scanned;
> >  
> > diff --git a/drivers/staging/r8188eu/core/rtw_mlme.c b/drivers/staging/
> r8188eu/core/rtw_mlme.c
> > index 3e9882f89f76..aed868d1d47b 100644
> > --- a/drivers/staging/r8188eu/core/rtw_mlme.c
> > +++ b/drivers/staging/r8188eu/core/rtw_mlme.c
> > @@ -61,7 +61,6 @@ static int _rtw_init_mlme_priv(struct adapter 
> *padapter)
> >  		res = _FAIL;
> >  		goto exit;
> >  	}
> > -	pmlmepriv->free_bss_buf = pbuf;
> 
> Hi Jaehee,
> 
> "pmlmepriv->free_bss_buf" is assigned with "pbuf". The latter is a pointer 
> to virtually contiguous memory which was allocated by vmalloc() or 
> vzalloc() (I didn't check, but the vfree() in _rtw_free_mlme_priv() tells 
> me that indeed it was).
> 
> >  	pnetwork = (struct wlan_network *)pbuf;
> >  
> > @@ -109,13 +108,7 @@ void rtw_free_mlme_priv_ie_data(struct mlme_priv 
> *pmlmepriv)
> >  
> >  void _rtw_free_mlme_priv(struct mlme_priv *pmlmepriv)
> >  {
> > -
> >  	rtw_free_mlme_priv_ie_data(pmlmepriv);
> > -
> > -	if (pmlmepriv) {
> > -		vfree(pmlmepriv->free_bss_buf);
> > -	}
> 
> Therefore, here you are causing a memory leak, which is something you 
> should avoid :) 
> 
> Why did you delete that call to vfree()? 
> 
> I think that you are misunderstanding what Pavel said. Even if it were true 
> that the code makes no use of that region of memory (again, I didn't 
> check), nevertheless, that memory was allocated somewhere and its address 
> is now in "pmlmepriv->free_bss_buf".
> 

My understanding of Pavel's response is the free_bss_buf member of the 
pmlmepriv structure wasn't being used anywhere and that the 
rtw_free_mlme_riv_ie_data function frees the memory of the pmlmepriv 
structure so the second check is redundant. 

However, as Fabio said, the free_bss_buf member is being used and pbuf 
memory is not being freed.
So I'll revert the patch as it was originally (which was just removing 
the {} around the single if statement). 

> If you can confirm that this memory is allocated for no purpose you should 
> also remove the call to vmalloc() / vzalloc().
> 
> Thanks,
> 
> Fabio M. De Francesco  
> 
> > -
> >  }
> >  
> >  struct	wlan_network *_rtw_alloc_network(struct	mlme_priv *pmlmepriv)/* 
> _queue *free_queue) */
> > -- 
> > 2.25.1
> >
> 
>