Re: [PATCH v7] livepatch: Clear relocation targets on a module removal

[Petr Mladek <pmladek@suse.com>]

On Fri 2023-01-06 14:02:27, Miroslav Benes wrote:
> Hi
> 
> On Thu, 5 Jan 2023, Joe Lawrence wrote:
> 
> > On 1/5/23 00:59, Song Liu wrote:
> > > On Wed, Jan 4, 2023 at 3:12 PM Joe Lawrence <joe.lawrence@redhat.com> wrote:
> > >>
> > >>
> > >> Stepping back, this feature is definitely foot-gun capable.
> > >> Kpatch-build would expect that klp-relocations would only ever be needed
> > >> in code that will patch the very same module that provides the
> > >> relocation destination -- that is, it was never intended to reference
> > >> through one of these klp-relocations unless it resolved to a live
> > >> module.
> > >>
> > >> On the other hand, when writing the selftests, verifying against NULL
> > >> [1] provided 1) a quick sanity check that something was "cleared" and 2)
> > >> protected the machine against said foot-gun.
> > >>
> > >> [1] https://github.com/joe-lawrence/klp-convert-tree/commit/643acbb8f4c0240030b45b64a542d126370d3e6c
> > > 
> > > I don't quite follow the foot-gun here. What's the failure mode?
> > > 
> > 
> > Kpatch-build, for better or worse, hides the potential problem.  A
> > typical kpatch scenario would be:
> > 
> > 1. A patch modifies module foo's function bar(), which references
> > symbols local to module foo
> > 
> > 2. Kpatch-build creates a livepatch .ko with klp-relocations in the
> > modified bar() to foo's symbols
> > 
> > 3. When loaded, modified bar() code that references through its
> > klp-relocations to module foo will only ever be active when foo is
> > loaded, i.e. when the original bar() redirects to the livepatch version.
> > 
> > However, writing source-based livepatches (like the kselftests) offers a
> > lot more freedom.  There is no implicit guarantee from (3) that the
> > module is loaded.  One could reference klp-relocations from anywhere in
> > the livepatch module.
> 
> Yes, on the other hand the approach you describe above seems to be the 
> only reasonable one in my opinion. The rest might be considered a bug. 
> Foot-gun as you say. I am not sure if we can do anything about it.

I agree that using an address from a module when the module is not
loaded is a bug. The kernel might still crash even when we clear
the addresses. But at least it can't be used to read information
from an "arbitrary" address.

It means that clearing the relocations is nice to have.

BTW: I originally understood the "foot-gun" in a more generic way.
     Modyfying "elf" sections feels like a surgery. As such, it has
     to be done carefully.

> > > [...]
> > > 
> > >>> These approaches don't look better to me. But I am ok
> > >>> with any of them. Please just let me know which one is
> > >>> most preferable:
> > >>>
> > >>> a. current version;
> > >>> b. clear_ undo everything of apply_ (the sample code
> > >>>    above)
> > >>> c. clear_ undo R_PPC_REL24, but _redo_ everything
> > >>>    of apply_ for other ELF64_R_TYPEs. (should be
> > >>>   clearer code than option b).
> > >>>
> > >>
> > >> This was my attempt at combining and slightly refactoring the power64
> > >> version.  There is so much going on here I was tempted to split off it
> > >> into separate value assignment and write functions.  Some changes I
> > >> liked, but I wasn't all too happy with the result.  Also, as you
> > >> mention, completely undoing R_PPC_REL24 is less than trivial... for this
> > >> arch, there are basically three major tasks:
> > >>
> > >>   1) calculate the new value, including range checking
> > >>   2) special constructs created by restore_r2 / create_stub
> > >>   3) writing out the value
> > >>
> > >> and many cases are similar, but subtly different enough to avoid easy
> > >> code consolidation.
> > > 
> > > Thanks for exploring this direction. I guess this part won't be perfect
> > > anyway.
> > > 
> > > PS: While we discuss a solution for ppc64, how about we ship the
> > > fix for other archs first? I think there are only a few small things to
> > > be addressed.
> > > 
> > 
> > Yeah, the x86_64 version looks a lot simpler and closer to being done.
> > Though I believe that Petr would prefer a complete solution, but I'll
> > let him speak to that.
> 
> I cannot speak for Petr, but I think it might be easier to split it given 
> the situation. Then we can involve arch maintainers for ppc64le because 
> they might have a preference with respect to a, b, c options above.
> 
> Petr, what do you think?

I am fine with solving each architecture in a separate patch or
patchset. It would make it easier, especially, for arch maintainers.

Best Regards,
Petr