Make sure each symbol address doesn't start with a jump instruction

[cormander]

This patch makes kpatch_link_object run through all the verify steps before it attempts to patch them. On failure, there is no need to call unlink (although unlink is called on the return path anyway - but I'll tackle that problem another day) so add the err_verify jump point.

We also now check the start of the symbol address for the 0xe9 OP_JMP_REL32 instruction. This may not be the best place to do it, but it's the soonest place I could find that I could add the check without doing any major rewrites.

The purpose of this check is to make sure the functions havne't been redirected outside of the normal kernel internal redirect systems, because if they have been, we'll run into problems down the road.

[cormander]

Hi guys,

An easy way to test the issue I'm solving in this patch is to build / install this module;

https://github.com/cormander/tpe-lkm

And then try to load a patch that modifies one of the hijacked functions;
do_mmap_pgoff
security_bprm_check
security_file_mprotect
proc_sys_write
pid_revalidate
m_show
kallsyms_open
security_task_fix_setuid

[ 72.295961] WARNING: at kernel/trace/ftrace.c:1663 ftrace_bug+0x25d/0x270()

[ 72.296059] ftrace failed to modify [] security_bprm_check+0x0/0x30

It's not that there is a bug in ftrace (or maybe ftrace ought to do this check instead of kpatch?) but there's a lot of stacktrace under this condition. My ultimate goal would be to have kpatch do the symbol checks / etc on all symbols in all changing objects before it attempts to load them; that way, when we encounter an error, we don't run into issues backing them out because they never went in in the first place. Checking for the jump at the symbol start address would be one such of those sanity checks.

I don't expect you to take this patch verbatim - I moved the call to kpatch_write_relocations() and I'm not sure if it needs to be called before the verification steps. I don't think so, but I haven't looked at this project's internals long enough to know for sure.

[jpoimboe]

This patch makes kpatch_link_object run through all the verify steps before it attempts to patch them. On failure, there is no need to call unlink (although unlink is called on the return path anyway - but I'll tackle that problem another day) so add the err_verify jump point.

Yeah, looks like the kpatch_link_object() error path is messed up. I'll open another issue for that.

We also now check the start of the symbol address for the 0xe9 OP_JMP_REL32 instruction. This may not be the best place to do it, but it's the soonest place I could find that I could add the check without doing any major rewrites.

I feel the same here as I do about kpatch_enabled. I'd much rather have ftrace properly return an error, instead of having kpatch be responsible for ftrace's error handling.

[jpoimboe]

Closing for now, see #385 for the current ftrace error handling plan.