linux-kernel - Re: [PATCH] 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: <YbtX088SeDWaEih1@dev0025.ash9.facebook.com>
Date:   Thu, 16 Dec 2021 07:14:27 -0800
From:   David Vernet <void@...ifault.com>
To:     Petr Mladek <pmladek@...e.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Miroslav Benes <mbenes@...e.cz>, linux-doc@...r.kernel.org,
        live-patching@...r.kernel.org, linux-kernel@...r.kernel.org,
        jpoimboe@...hat.com, jikos@...nel.org, joe.lawrence@...hat.com,
        corbet@....net, yhs@...com, songliubraving@...com
Subject: Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path

Petr Mladek <pmladek@...e.com> wrote on Thu [2021-Dec-16 15:14:38 +0100]:
> > Now it does.  In the past, I think we did create some memory.  I know
> > when we hook debugobjects up to kobjects (there's an external patch for
> > that floating around somewhere), that is one reason to keep the
> > kobject_put() rule, and there might have been other reasons in the past
> > 20+ years as well.
> > 
> > So yes, while you are correct today, the "normal" reference counted
> > object model patern is "after the object is initialized, it MUST only be
> > freed by handling its reference count."  So let's stick to that rule for
> > now.
> 
> Good point.

Thanks for the discussion all. I think we've landed on the fact that this
is a refcounting bug that needs to be fixed, but isn't a leak in how the
kobject implementation exists today.

Petr - are you OK with me sending out a v3 of the patch with the following
changes:
  - The patch description is updated to not claim that a leak is being
    fixed, but rather that a kobject reference counting bug is being fixed.
  - All of the NULL checking in klp_init_patch_early() is brought into
    klp_enable_patch(), and klp_init_patch_early() is updated to be void,
    per Josh's suggestion. This would address the refcounting issue and IMO
    also simplifies and improves the code. I know you were onboard with
    moving try_module_get() into klp_enable_patch(), but I don't think we
    ever resolved the point about moving the rest of the NULL checking out
    as well.

Regards,
David