linux-kernel - Re: [PATCH 6/7] livepatch: Remove 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:   Thu, 16 Apr 2020 11:28:25 +0200 (CEST)
From:   Miroslav Benes <mbenes@...e.cz>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
cc:     Jessica Yu <jeyu@...nel.org>, live-patching@...r.kernel.org,
        linux-kernel@...r.kernel.org, Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH 6/7] livepatch: Remove module_disable_ro() usage

On Wed, 15 Apr 2020, Josh Poimboeuf wrote:

> On Wed, Apr 15, 2020 at 05:02:16PM +0200, Jessica Yu wrote:
> > +++ Josh Poimboeuf [14/04/20 11:28 -0500]:
> > > With arch_klp_init_object_loaded() gone, and apply_relocate_add() now
> > > using text_poke(), livepatch no longer needs to use module_disable_ro().
> > > 
> > > The text_mutex usage can also be removed -- its purpose was to protect
> > > against module permission change races.
> > > 
> > > Signed-off-by: Josh Poimboeuf <jpoimboe@...hat.com>
> > > ---
> > > kernel/livepatch/core.c | 8 --------
> > > 1 file changed, 8 deletions(-)
> > > 
> > > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > > index 817676caddee..3a88639b3326 100644
> > > --- a/kernel/livepatch/core.c
> > > +++ b/kernel/livepatch/core.c
> > > @@ -767,10 +767,6 @@ static int klp_init_object_loaded(struct klp_patch *patch,
> > > 	struct klp_modinfo *info = patch->mod->klp_info;
> > > 
> > > 	if (klp_is_module(obj)) {
> > > -
> > > -		mutex_lock(&text_mutex);
> > > -		module_disable_ro(patch->mod);
> > > -
> > 
> > Don't you still need the text_mutex to use text_poke() though?
> > (Through klp_write_relocations -> apply_relocate_add -> text_poke)
> > At least, I see this assertion there:
> > 
> > void *text_poke(void *addr, const void *opcode, size_t len)
> > {
> > 	lockdep_assert_held(&text_mutex);
> > 
> > 	return __text_poke(addr, opcode, len);
> > }
> 
> Hm, guess I should have tested with lockdep ;-)

:)

If I remember correctly, text_mutex must be held whenever text is modified 
to prevent race due to the modification. It is not only about permission 
changes even though it was how it manifested itself in our case.

Miroslav