Re: [RFC PATCH 2/2] livepatch: Clear relocation targets on a module removal

From: Josh Poimboeuf
Date: Tue Aug 27 2019 - 11:37:59 EST


On Tue, Aug 27, 2019 at 11:05:51AM -0400, Joe Lawrence wrote:
> > Sure, it introduces risk. But we have to compare that risk (which only
> > affects rare edge cases) with the ones introduced by the late module
> > patching code. I get the feeling that "late module patching" introduces
> > risk to a broader range of use cases than "occasional loading of unused
> > modules".
> >
> > The latter risk could be minimized by introducing a disabled state for
> > modules - load it in memory, but don't expose it to users until
> > explicitly loaded. Just a brainstormed idea; not sure whether it would
> > work in practice.
> >
>
> Interesting idea. We would need to ensure consistency between the
> loaded-but-not-enabled module and the version on disk. Does module init run
> when it's enabled? Etc.

I don't think there can be a mismatch unless somebody is mucking with
the .ko files directly -- and anyway that would already be a problem
today, because the patch module already assumes that the runtime version
of the module matches what the patch module was built against.

> <blue sky ideas>
>
> What about folding this the other way? ie, if a livepatch targets unloaded
> module foo, loaded module bar, and vmlinux ... it effectively patches bar
> and vmlinux, but the foo changes are dropped. Responsibility is placed on
> the admin to install an updated foo before loading it (in which case,
> livepatching core will again ignore foo.)
>
> Building on this idea, perhaps loading that livepatch would also blacklist
> specific, known vulnerable (unloaded) module versions. If the admin tries
> to load one, a debug msg is generated explaining why it can't be loaded by
> default.
>
> </blue sky ideas>

I like this.

One potential tweak: the updated modules could be delivered with the
patch module, and either replaced on disk or otherwise hooked into
modprobe.

> > > > > + It might open more security holes that are not fixed by
> > > > > the livepatch.
> > > >
> > > > Following the same line of thinking, the livepatch infrastructure might
> > > > open security holes because of the inherent complexity of late module
> > > > patching.
> > >
> > > Could you be more specific, please?
> > > Has there been any known security hole in the late module
> > > livepatching code?
> >
> > Just off the top of my head, I can think of two recent bugs which can be
> > blamed on late module patching:
> >
> > 1) There was a RHEL-only bug which caused arch_klp_init_object_loaded()
> > to not be loaded. This resulted in a panic when certain patched code
> > was executed.
> >
> > 2) arch_klp_init_object_loaded() currently doesn't have any jump label
> > specific code. This has recently caused panics for patched code
> > which relies on static keys. The workaround is to not use jump
> > labels in patched code. The real fix is to add support for them in
> > arch_klp_init_object_loaded().
> >
> > I can easily foresee more problems like those in the future. Going
> > forward we have to always keep track of which special sections are
> > needed for which architectures. Those special sections can change over
> > time, or can simply be overlooked for a given architecture. It's
> > fragile.
>
> FWIW, the static keys case is more involved than simple deferred relocations
> -- those keys are added to lists and then the static key code futzes with
> them when it needs to update code sites. That means the code managing the
> data structures, kernel/jump_label.c, will need to understand livepatch's
> strangely loaded-but-not-initialized variants.
>
> I don't think the other special sections will require such invasive changes,
> but it's something to keep in mind with respect to late module patching.

Maybe it could be implemented in a way that such differences are
transparent (insert lots of hand-waving).

So as far as I can tell, we currently have three feasible options:

1) drop unloaded module changes (and blacklist the old .ko and/or replace it)
2) use per-object patches (with no exported function changes)
3) half-load unloaded modules so we can patch them

I think I like #1, if we could figure out a simple way to do it.

--
Josh