linux-kernel - Re: [PATCH] hfs: fix a memleak in hfs_find

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] [day] [month] [year] [list]

Message-ID: <44665a1c.22c9.18d55cbff19.Coremail.alexious@zju.edu.cn>
Date: Mon, 29 Jan 2024 23:17:28 +0800 (GMT+08:00)
From: alexious@....edu.cn
To: "Viacheslav Dubeyko" <slava@...eyko.com>
Cc: "Andrew Morton" <akpm@...ux-foundation.org>,
	"Desmond Cheong Zhi Xi" <desmondcheongzx@...il.com>,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] hfs: fix a memleak in hfs_find_init

> > On 29 Jan 2024, at 15:54, alexious@....edu.cn wrote:
> > 
> >>> On 22 Jan 2024, at 20:27, Zhipeng Lu <alexious@....edu.cn> wrote:
> >>> 
> >>> When the switch statment goes to default and return an error, ptr should
> >>> be freed since it is allocated in hfs_find_init.
> >>> 
> >> 
> >> Do you have any memory leaks report? Could you share it in the comments?
> >> Which use-case reproduces the issue? It will be easier to review the fix
> >> If you can share the path of reproduction.
> >> 
> >> Thanks,
> >> Slava.
> > 
> > Well, we found this potential memory leak by static analysis.
> > 
> > We found that all of hfs_find_init's callers won't release `ptr` when 
> > hfs_find_init fails, while they will do release `ptr` when functions 
> > that after hfs_find_init fails. This tactic observation suggests that
> > hfs_find_init proberly should release `ptr` when it fails, i.e. in the
> > default branch of switch in this patch.
> > 
> > Besides, we noticed another implementation of hfs_find_init in 
> > fs/hfsplus/bfind.c, which is essentially identical to the one in 
> > this patch (in fs/hfs/bfind.c) but calling `BUG();` in default branch
> > to trigger an error-handling.
> > 
> 
> I see. I believe it makes sense to add all of this explanation
> into comment section. Modification looks good. Mostly, hfs_find_exit()
> does freeing resources and if hfs_find_init() fails, then hfs_find_exit()
> is never called. Maybe, it makes sense to set fd->tree = NULL too but
> it is not critical, as far as I can see.
> 
> Could you please rework the comment section of the patch? 

Sure, I'll including such idea in this patch and send a v2 version of 
this patch.

Thanks,
Zhipeng.

> Thanks,
> Slava.