linux-kernel - Re: [PATCH][next] fs: dlm: Fix memory leak of object mh

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: <20210526181726.GJ24442@kadam>
Date:   Wed, 26 May 2021 21:24:16 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Colin Ian King <colin.king@...onical.com>
Cc:     Christine Caulfield <ccaulfie@...hat.com>,
        David Teigland <teigland@...hat.com>,
        Alexander Aring <aahringo@...hat.com>,
        cluster-devel@...hat.com, kernel-janitors@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH][next] fs: dlm: Fix memory leak of object mh

On Wed, May 26, 2021 at 04:11:06PM +0100, Colin Ian King wrote:
> On 26/05/2021 16:01, Dan Carpenter wrote:
> > On Wed, May 26, 2021 at 02:40:39PM +0100, Colin King wrote:
> >> From: Colin Ian King <colin.king@...onical.com>
> >>
> >> There is an error return path that is not kfree'ing mh after
> >> it has been successfully allocates.  Fix this by free'ing it.
> >>
> >> Addresses-Coverity: ("Resource leak")
> >> Fixes: a070a91cf140 ("fs: dlm: add more midcomms hooks")
> >> Signed-off-by: Colin Ian King <colin.king@...onical.com>
> >> ---
> >>  fs/dlm/rcom.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/fs/dlm/rcom.c b/fs/dlm/rcom.c
> >> index 085f21966c72..19298edc1573 100644
> >> --- a/fs/dlm/rcom.c
> >> +++ b/fs/dlm/rcom.c
> >> @@ -393,6 +393,7 @@ static void receive_rcom_lookup(struct dlm_ls *ls, struct dlm_rcom *rc_in)
> >>  	if (rc_in->rc_id == 0xFFFFFFFF) {
> >>  		log_error(ls, "receive_rcom_lookup dump from %d", nodeid);
> >>  		dlm_dump_rsb_name(ls, rc_in->rc_buf, len);
> >> +		kfree(mh);
> > 
> > Am I looking at the same code as you?  (I often am not able to review
> > your patches because you're doing development on stuff that hasn't hit
> > linux-next).  Anyway, to me this doesn't seem like the correct fix at
> > all.  There are some other things to free and the "mh" pointer is on
> > a bunch of lists so it leads to use after frees.
      ^^^^^^^^^^^^^^
This is sort of impossible, of course, because the struct only has one
list_head so it can only be in one list and not a "bunch of lists".

The dlm code seems to be going out of its way to use void pointers and
that makes it difficult to parse with Smatch.

But in other subsystems, we could make it a rule that list_heads are
"poison" "init" or "added".  If you freed a memory with an "added"
list_head then print a warning.  Or if you added a list_head but it was
already in the added state then print a warning.  Another idea is that
if you freed a struct mh before the mh->page allocation was freed then
print a warning about the leak.  This one is probably more prone to
false positives but there might be workarounds for those.  #IdeasToImplement

regards,
dan carpenter