linux-ext4 - Re: [PATCH v3] setup

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]

Message-ID: <Y8eC4uM9b3hiBpfR@mit.edu>
Date:   Wed, 18 Jan 2023 00:25:54 -0500
From:   "Theodore Ts'o" <tytso@....edu>
To:     zhanchengbin <zhanchengbin1@...wei.com>
Cc:     linux-ext4@...r.kernel.org, liuzhiqiang26@...wei.com,
        linfeilong@...wei.com, riteshh <riteshh@...ux.ibm.com>
Subject: Re: [PATCH v3] setup_tdb : fix memory leak

On Tue, Jan 04, 2022 at 08:09:16PM +0800, zhanchengbin wrote:
> In setup_tdb(), need free tdb_dir and db->tdb_fn before return,
> otherwise it will cause memory leak.

This patch is not correct.  tdb_dir is returned by
profile_get_string(), and it does *not* need to be freed.  In fact, if
you had tested this using valgrind or AddressSanitizer, it would have
failed because tdb_dir is in the *middle* of an allocated block[1],
and this would have corrupted the heap data structures leading to all
sorts of potential problems.

[1] And that allocated block is freed by profile_release()

In addition, tdb->tdb_fn will be freed by e2fsck_free_dir_info(), and
so freeing it on the error path in setup_tdb() will result in a
double-free when e2fsck_free_dir_info() gets called.

I'm guessing you didn't actually test this patch with the code paths
in question --- that is, by triggering an error while using something
like ASan or valgrid?  Note that corrupting the heap may lead to an
exploitable security bug, so if you have applied this patch in your
production version of e2fsprogs, I suggest that you revert it.

Cheers,

					- Ted