linux-kernel - Re: [PATCH v2] livepatch: Fix leak on klp_init_patch

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]

Date:   Mon, 20 Dec 2021 10:48:41 +0100
From:   Petr Mladek <pmladek@...e.com>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     David Vernet <void@...ifault.com>, live-patching@...r.kernel.org,
        linux-kernel@...r.kernel.org, jikos@...nel.org, mbenes@...e.cz,
        joe.lawrence@...hat.com, corbet@....net, songliubraving@...com,
        gregkh@...uxfoundation.org
Subject: Re: [PATCH v2] livepatch: Fix leak on klp_init_patch_early failure
 path

On Fri 2021-12-17 13:50:42, Josh Poimboeuf wrote:
> On Fri, Dec 17, 2021 at 02:51:42PM +0100, Petr Mladek wrote:
> > On Wed 2021-12-15 07:20:04, David Vernet wrote:
> > Josh's proposal adds pre-early-init() to allow calling
> > klp_free_patch_*() already when klp_init_*_early() fails.
> > The purpose is to make sure that klp_init_*_early()
> > will actually never fail.
> > 
> > This might make things somehow complicated. Any future change
> > in klp_init_*_early() might require change in pre-early-init()
> > to catch eventual problems earlier.
> 
> I'm not sure why that would be a problem.  If we can separate out the
> things which fail from the things which don't, it simplifies things.

I think that there is no right answer. It depends on personal
preferences.


> > My proposal is to use simple trick. klp_free_patch_*() iterate
> > using the dynamic list (list_for_each_entry) while klp_init_*_early()
> > iterate using the arrays.
> > 
> > Now, we just need to make sure that klp_init_*_early() will only add
> > fully initialized structures into the dynamic list. As a result,
> > klp_free_patch() will see only the fully initialized structures
> > and could release them. It will not see that not-yet-initialized
> > structures but it is fine. They are not initialized and they do not
> > need to be freed.
> > 
> > In fact, I think that klp_init_*_early() functions already do
> > the right thing. IMHO, if you move the module_get() then you
> > could safely do:
> > 
> > int klp_enable_patch(struct klp_patch *patch)
> > {
> > [...]
> > 	if (!try_module_get(patch->mod)) {
> > 		mutex_unlock(&klp_mutex);
> > 		return -ENODEV;
> > 	}
> > 
> > 	ret = klp_init_patch_early(patch);
> > 	if (ret)
> > 		goto err;
> > 
> > 
> > Note that it would need to get tested.
> > 
> > Anyway, does it make sense now?
> 
> It would work, but it's too clever/non-obvious.  If we rely on tricks
> then we may eventually forget about them and accidentally break
> assumptions later.

It is not that super magic from my POV. But again, it is a personal
taste.

I do not want to bikeshed about it. Feel free to use the pre-early
thing.

Best Regards,
Petr