[kpatch] Correlating unchanged locals

[Evgenii Shatokhin]

On 14.10.2016 20:49, Josh Poimboeuf wrote:
 > On Fri, Oct 14, 2016 at 04:21:59PM +0300, Evgenii Shatokhin wrote:
 >> Hi,
 >>
 >> It might be not a problem in Kpatch itself but perhaps you could give an
 >> advice on how to deal with it.
 >>
 >> I hit a strange problem when experimenting with the patches for
 >> CVE-2015-7872 and CVE-2016-5696 for the kernel 3.10.0-327.4.4 in CentOS.
 >>
 >> To build the binary patch for these, I used the same GCC as was used 
for the
 >> kernel, GCC 4.8.3 20140911 (Red Hat 4.8.3-9).
 >>
 >> The following error was reported by kpatch-build:
 >>
 >> /usr/libexec/kpatch/create-diff-object: ERROR: gc.o:
 >> kpatch_create_dynamic_rela_sections: 2659: lookup_local_symbol
 >> graveyard.20319 (gc.c) needed for .text.key_gc_unused_keys.constprop.1
 >>
 >> The kernel has such symbol but with a different numeric suffix:
 >> $ readelf -sW ./vmlinux | grep -F graveyard.
 >>   24328: ffffffff819df280    16 OBJECT  LOCAL  DEFAULT   15 
graveyard.20316
 >>
 >> I cannot say why the same GCC behaved differently in these cases.
 >>
 >> I can change lookup_local_symbol() so that it would ignore such 
suffixes for
 >> variables (but not for the functions) when matching the names. This 
is not
 >> enough however, because the dynrela for that symbol still refers to
 >> graveyard.20319 and the binary patch fails to load as a result.
 >>
 >> The problem has not shown up for other kernels so far, only for
 >> 3.10.0-327.4.4.
 >>
 >> Any ideas?
 >
 > Hi Evgenii,
 >
 > gcc added the suffix to the graveyard variable because it's a static
 > local variable, which is very similar to a global variable, except its
 > scope is local to the function.  So it's possible for there to be
 > several of these variables with the same name in the same file, or even
 > in the same function.
 >
 > The .20316 suffix is its gcc object number, which can change even if you
 > patch completely unrelated code.  It's always added to static local
 > variables to distinguish them for other static locals which might have
 > the same name.

Yes, I know about this peculiarity of GCC.

 >
 > In general, dealing with static local variables properly is hard, and
 > has been a real problem area for kpatch-build.  We have plans to rewrite
 > the static local handling code (yet again).  For some background, see:
 > https://github.com/dynup/kpatch/issues/545
 >
 > Anyway, I don't know what specific issue you're seeing, but feel free to
 > open a github issue for it.
 >

While experimenting with that kernel more, I encountered a more serious 
issue, which can be related to static locals. The binary patch for 
CVE-2016-3134 builds without errors but fails to load on that kernel.

Reported it here: https://github.com/dynup/kpatch/issues/612

Regards,
Evgenii