linux-kernel - Re: [PATCH v2 3/3] livepatch: Cleanup module page permission changes

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: <20151106121247.GF28254@treble.redhat.com>
Date:	Fri, 6 Nov 2015 06:12:47 -0600
From:	Josh Poimboeuf <jpoimboe@...hat.com>
To:	Petr Mladek <pmladek@...e.com>
Cc:	Seth Jennings <sjenning@...hat.com>,
	Jiri Kosina <jikos@...nel.org>,
	Vojtech Pavlik <vojtech@...e.com>,
	linux-kernel@...r.kernel.org, live-patching@...r.kernel.org,
	"Cyril B." <cbay@...aysdata.com>,
	Rusty Russell <rusty@...tcorp.com.au>,
	Miroslav Benes <mbenes@...e.cz>
Subject: Re: [PATCH v2 3/3] livepatch: Cleanup module page permission changes

On Fri, Nov 06, 2015 at 11:40:55AM +0100, Petr Mladek wrote:
> On Thu 2015-11-05 15:18:05, Josh Poimboeuf wrote:
> > Calling set_memory_rw() and set_memory_ro() for every iteration of the
> > loop in klp_write_object_relocations() is messy, inefficient, and
> > error-prone.
> > 
> > Change all the read-only pages to read-write before the loop and convert
> > them back to read-only again afterwards.
> > 
> > The {un}set_module_core_ro_nx() functions are used to change the
> > page permissions.  Toggling NX isn't necessary in this case, but it's
> > not highly performance sensitive code so it should be fine.
> 
> Hmm, the name (un)set_module_core_ro_nx() still sounds a bit strange,
> especially the "ro_nx" suffix.

> Alternative solution would be to create
> 
>    set_module_text_rw()
>    set_module_text_ro()
> 
> There already exists
> 
>    set_all_modules_text_rw()
>    set_all_modules_text_ro()
> 
> They modify only the ro/rw flags. IMHO, the name is more descriptive
> They are used by ftrace for very similar purpose.

That wouldn't be enough.  Relocations can occur not only in text, but
also in data.  That includes read-only data.

The (un)set_module_core_ro_nx() naming was taken from the names of
existing module functions (unset_module_{core,init}_ro_nx()).  They
enable/disable the CONFIG_DEBUG_SET_MODULE_RONX feature on the core part
of the module.  The name makes sense to me, though I'm certainly open to
other ideas.

> They modify also the init section. But we might want to touch it
> as well. klp_module_notify() is called too late now. But once we
> have a more complex consistency model, we will need to reject
> the module when the patching fails. We will need to call the
> livepatch init earlier, close to ftrace_module_init(mod).
> Then the init section might be interesting as well.

Init section functions don't have the __fentry() call, so they can't be
patched.  If that were to change in the future, we could use the
(un)set_module_init_ro_nx() functions, which already exist.

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/