linux-kernel - Re: [PATCH 0/7] livepatch,module: Remove .klp.arch and module_disable

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]

Date:   Fri, 17 Apr 2020 10:27:04 +0200 (CEST)
From:   Miroslav Benes <mbenes@...e.cz>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
cc:     Jessica Yu <jeyu@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        live-patching@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/7] livepatch,module: Remove .klp.arch and
 module_disable_ro()

On Thu, 16 Apr 2020, Josh Poimboeuf wrote:

> On Thu, Apr 16, 2020 at 05:31:31PM +0200, Jessica Yu wrote:
> > > But I still not a fan of the fact that COMING has two different
> > > "states".  For example, after your patch, when apply_relocate_add() is
> > > called from klp_module_coming(), it can use memcpy(), but when called
> > > from klp module init() it has to use text poke.  But both are COMING so
> > > there's no way to look at the module state to know which can be used.
> > 
> > This is a good observation, thanks for bringing it up. I agree that we
> > should strive to be consistent with what the module states mean. In my
> > head, I think it is easiest to assume/establish the following meanings
> > for each module state:
> > 
> > MODULE_STATE_UNFORMED - no protections. relocations, alternatives,
> > ftrace module initialization, etc. any other text modifications are
> > in the process of being applied. Direct writes are permissible.
> > 
> > MODULE_STATE_COMING - module fully formed, text modifications are
> > done, protections applied, module is ready to execute init or is
> > executing init.
> > 
> > I wonder if we could enforce the meaning of these two states more
> > consistently without needing to add another module state.
> > 
> > Regarding Peter's patches, with the set_all_modules_text_*() api gone,
> > and ftrace reliance on MODULE_STATE_COMING gone (I think?), is there
> > anything preventing ftrace_module_init+enable from being called
> > earlier (i.e., before complete_formation()) while the module is
> > unformed? Then you don't have to move module_enable_ro/nx later and we
> > keep the MODULE_STATE_COMING semantics. And if we're enforcing the
> > above module state meanings, I would also be OK with moving jump_label
> > and static_call out of the coming notifier chain and making them
> > explicit calls while the module is still writable.
> > 
> > Sorry in advance if I missed anything above, I'm still trying to wrap
> > my head around which callers need what module state and what module
> > permissions :/
> 
> Sounds reasonable to me...
> 
> BTW, instead of hard-coding the jump-label/static-call/ftrace calls, we
> could instead call notifiers with MODULE_STATE_UNFORMED.

That was exactly what I was thinking about too while reading Jessica's 
email. Since (hopefully all if I remember correctly. I checked only 
random subset now) existing module notifiers check module state, 
it should not be a problem.

Miroslav