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

Message-ID: <YboHpHmu3D+0hxKp@dev0025.ash9.facebook.com>
Date:   Wed, 15 Dec 2021 07:20:04 -0800
From:   David Vernet <void@...ifault.com>
To:     Petr Mladek <pmladek@...e.com>
Cc:     Josh Poimboeuf <jpoimboe@...hat.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

Petr Mladek <pmladek@...e.com> wrote on Wed [2021-Dec-15 11:06:15 +0100]:
> Well, I still believe that this is just a cargo cult. And I would prefer
> to finish the discussion about it, first, see
> https://lore.kernel.org/all/YbmlL0ZyfSuek9OB@alley/

No problem, I won't send out v3 until we've finished the discussion and
have consensus. I'll assume that the discussion on whether or not there is
a leak will continue on the thread you linked to above, so I won't comment
on it here.

> Note that klp_init_*_early() functions iterate through the arrays
> using klp_for_each_*_static. While klp_free_*() functions iterate
> via the lists using klp_for_each_*_safe().

Correct, as I've understood it, klp_for_each_*_safe() should only iterate
over the objects that have been added to the patch and klp_object's lists,
and thus for which kobject_init() has been invoked. So if we fail a check
on 'struct klp_object' N, then we'll only iterate over the first N - 1
objects in klp_for_each_*_safe().

> We should not need the pre-early-init check when the lists include only
> structures with initialized kobjects.

Not sure I quite follow. We have to do NULL checks for obj->funcs at some
point, and per Josh's suggestion it seems cleaner to do it outside the
critical section, and before we actually invoke kobject_init(). Apologies
if I've misunderstood your point.

> Otherwise, I like the idea to do module_get() before
> klp_init_patch_early(). I was never happy with the "hidden"
> side effect.

Ack!

> I am also fine with calling klp_free() when the early init fails
> if we agreed that it is a good practice. I just do want to pretend
> that it fixes a leak what nobody sees any leak.
> 
> Please, wait few days until the discussion finishes before sending v3.

Ack, no problem, I'll wait until we're all in alignment. Thanks, Petr and
Josh for taking a look at the patch.

Regards,
David