symbol load_elf_binary at offset 3536 within section .text.load_elf_binary, expected 0

[joe-lawrence]

patch.txt

I'm testing a patch (see attached) for CVE-2015-1593 against RHEL7.3 GA + kernel-3.10.0-200.el7.x86_64 (rebuilt by RHEL7.3 GA toolset) and was running into the following kpatch-build error:

Skipping cleanup
Fedora/Red Hat distribution detected
Downloading kernel source for 3.10.0-200.el7.x86_64
Unpacking kernel source
Testing patch file
checking file arch/x86/mm/mmap.c
checking file fs/binfmt_elf.c
Reading special section data
Building original kernel
Building patched kernel
Extracting new and modified ELF sections
mmap.o: changed function: stack_maxrandom_size
mmap.o: changed function: arch_pick_mmap_layout
binfmt_elf.o: changed function: load_elf_binary
compat_binfmt_elf.o: changed function: load_elf_binary
Patched objects: vmlinux
Building patch module: kpatch-klp.ko
/usr/local/libexec/kpatch/create-kpatch-module: ERROR: tmp_output.o: kpatch_create_symbol_list: 331: symbol load_elf_binary at offset 3536 within section .text.load_elf_binary, expected 0
ERROR: kpatch build failed. Check /root/.kpatch/build.log for more details.

It's interesting to note that kpatch-build reports that both binfmt_elf.o and compat_binfmt_elf.o modify a function called load_elf_binary. (Check out fs/compat_binfmt_elf.c for it #include's binfmt_elf.c!) I think kpatch-build is getting tripped up on these duplicate functions as:

TMP=~/.kpatch/tmp/patch/tmp_output.o
readelf --wide --symbols $TMP | grep load_elf_binary
Symbol table '.symtab' contains 52 entries:
   Num:    Value          Size Type    Bind   Vis      Ndx Name
   ...
    24: 0000000000000000  3523 FUNC    LOCAL  DEFAULT    5 load_elf_binary
    26: 0000000000000dd0  3656 FUNC    LOCAL  DEFAULT    5 load_elf_binary

Note that the first load_elf_binary symbol value is 0 and the second is 0xdd0 = 3536, the found and expected values in the kpatch-build complaint.

[joe-lawrence]

patch.txt

I can generate a similar "symbol_get_entries offset error" with the mega-patch attached. This time, three files modify their own version of compat_get_entries():

% grep compat_get_entries kpatch-build2.out                                                                        
ip6_tables.o: changed function: compat_get_entries
ip_tables.o: changed function: compat_get_entries
arp_tables.o: changed function: compat_get_entries
/usr/local/libexec/kpatch/create-kpatch-module: ERROR: tmp_output.o: kpatch_create_symbol_list: 331: symbol compat_get_entries at offset 816 within section .text.compat_get_entries, expected 0

% TMP=~/.kpatch/tmp/patch/tmp_output.o
% readelf --wide --symbols $TMP | grep compat_get_entries
    49: 0000000000000000   801 FUNC    LOCAL  DEFAULT    5 compat_get_entries
    60: 0000000000000330   745 FUNC    LOCAL  DEFAULT    5 compat_get_entries
    79: 0000000000000620   722 FUNC    LOCAL  DEFAULT    5 compat_get_entries

[joe-lawrence]

This could probably be worked around by modifying the patches to provide a new, uniquely named function in each file that is modifying a commonly named function. For example, the patch in the previous comment could provide patched versions of compat_get_entries() prefixed by the containing filename. Callers would need to be updated to the new names as well.

Update: well maybe. If those functions contain local statics that need to be correlated from orig->patched, then obviously you can't rename the function.

[jpoimboe]

I saw this problem again today when patching fs/binfmt_elf.c.

Extracting new and modified ELF sections
binfmt_elf.o: new function: total_mapping_size
binfmt_elf.o: new function: elf_map
binfmt_elf.o: changed function: load_elf_interp.constprop.10
binfmt_elf.o: changed function: load_elf_binary
compat_binfmt_elf.o: new function: total_mapping_size
compat_binfmt_elf.o: new function: elf_map.isra.10
compat_binfmt_elf.o: changed function: load_elf_interp.constprop.11
compat_binfmt_elf.o: changed function: load_elf_binary
Patched objects: vmlinux
/root/kpatch/kpatch-build/create-kpatch-module: ERROR: tmp_output.o: kpatch_create_symbol_list: 376: symbol total_mapping_size at offset 103 within section .text.unlikely.total_mapping_size, expected 0

The problem is that fs/binfmt_elf.c is included in two separate objects. Each object has a total_mapping_size function which is bundled in a text.unlikely.total_mapping_size section. When those objects are linked into a single object, the two functions are concatenated into a single text.unlikely.total_mapping_size section. This breaks kpatch-build's one-function-per-section assumption for bundled functions.

[jpoimboe]

I think the one-function-per-section assumption for bundled functions is only important for create-diff-object. It shouldn't matter for create-kpatch-module. So we should probably move that check into create-diff-object instead of having it in the kpatch-elf library.