kpatch-build: incorrect or missing correlation of references to .rodata.*.str1_1 leads to kernel crashes #1225

euspectre · 2021-09-24T16:43:13Z

kpatch: 0.9.4
OS: CentOS 8 x86_64
Kernel: 4.18.0-305.19.1.el8_4.x86_64

While investigating an unrelated bug, I found that kpatch-build/create-diff-object probably does not correlate references to strings from .rodata.*.str1.1 properly and that causes difficult to debug kernel crashes.

Consider the following simple patch for sctp:

diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
index bfe3c26d1..9f9691063 100644
--- a/net/sctp/sysctl.c
+++ b/net/sctp/sysctl.c
@@ -370,6 +370,9 @@ static int proc_sctp_do_hmac_alg(struct ctl_table *ctl, int write,
 
 	memset(&tbl, 0, sizeof(struct ctl_table));
 
+	pr_info("[TEST] %s\n",
+		net->sctp.sctp_hmac_alg ? : none);
+
 	if (write) {
 		tbl.data = tmp;
 		tbl.maxlen = sizeof(tmp);

Note that, the locations of strings "md5" and "sha1" in .rodata.proc_sctp_do_hmac_alg.str1.1 changed in the patched code, because the printk format string was added:

[root@8176a445a8da centos8]# readelf -p .rodata.proc_sctp_do_hmac_alg.str1.1 kpatch/tmp/orig/net/sctp/sysctl.o       
String dump of section '.rodata.proc_sctp_do_hmac_alg.str1.1':
  [     0]  none
  [     5]  md5
  [     9]  sha1

[root@8176a445a8da centos8]# readelf -p .rodata.proc_sctp_do_hmac_alg.str1.1 kpatch/tmp/patched/net/sctp/sysctl.o
String dump of section '.rodata.proc_sctp_do_hmac_alg.str1.1':
  [     0]  none
  [     6]  6sctp: [TEST] %s\n
  [    18]  md5
  [    1c]  sha1

The strings are used as follows in proc_sctp_do_hmac_alg():

#ifdef CONFIG_CRYPTO_MD5
		if (!strncmp(tmp, "md5", 3)) {
			net->sctp.sctp_hmac_alg = "md5";
			changed = true;
		}
#endif
#ifdef CONFIG_CRYPTO_SHA1
		if (!strncmp(tmp, "sha1", 4)) {
			net->sctp.sctp_hmac_alg = "sha1";
			changed = true;
		}
#endif

Although this part of the source code remains unchanged, the binary code of the patched file is different because it refers to different locations in .rodata.proc_sctp_do_hmac_alg.str1.1.

The patch module has its own "md5" and "sha1" strings now, and the patched proc_sctp_do_hmac_alg() refers to them rather than to the original ones.

This is problematic because the addresses of the strings are stored in net->sctp.sctp_hmac_alg for later use. If we unload the patch module at the appropriate moment, the kernel will still keep the dangling addresses of these strings in the patch module and will crash when it tries to access them.

[root@centos8 temp]# kpatch load ./kpatch-sctp-str.ko 
loading patch module: ./kpatch-sctp-str.ko

[root@centos8 temp]# cat /sys/module/kpatch_sctp_str/sections/.rodata.proc_sctp_do_hmac_alg.str1.1
ffffffffc075f5f5

[root@centos8 temp]# modprobe sctp

[root@centos8 temp]# cat /proc/sys/net/sctp/cookie_hmac_alg
sha1

[root@centos8 temp]# echo md5 > /proc/sys/net/sctp/cookie_hmac_alg

[root@centos8 temp]# cat /proc/sys/net/sctp/cookie_hmac_alg
md5

[root@centos8 temp]# kpatch unload --all
disabling patch module: kpatch_sctp_str
waiting (up to 15 seconds) for patch transition to complete...
transition complete (2 seconds)
unloading patch module: kpatch_sctp_str

[root@centos8 temp]# cat /proc/sys/net/sctp/cookie_hmac_alg
[kernel crashed]

From vmcore-dmesg.txt:

[  143.827410] kpatch_sctp_str: loading out-of-tree module taints kernel.
[  143.827433] kpatch_sctp_str: tainting kernel with TAINT_LIVEPATCH
[  143.827474] kpatch_sctp_str: module verification failed: signature and/or required key missing - tainting kernel
[  143.827701] livepatch: enabling patch 'kpatch_sctp_str'
[  143.827752] livepatch: 'kpatch_sctp_str': starting patching transition
[  143.828698] livepatch: 'kpatch_sctp_str': patching complete
[  158.874261] livepatch: applying patch 'kpatch_sctp_str' to loading module 'sctp'
[  158.877213] sctp: Hash tables configured (bind 256/256)
[  189.311978] sctp: [TEST] sha1
[  189.312009] sctp: [TEST] sha1
[  212.399190] sctp: [TEST] sha1
[  232.559653] sctp: [TEST] md5
[  232.559677] sctp: [TEST] md5
[  250.031148] livepatch: 'kpatch_sctp_str': starting unpatching transition
[  251.565087] livepatch: 'kpatch_sctp_str': unpatching complete
[  260.230959] BUG: unable to handle kernel paging request at ffffffffc075f60d
[  260.230983] PGD 3d013067 P4D 3d013067 PUD 3d015067 PMD 16ea60067 PTE 0
[  260.230997] Oops: 0000 [#1] SMP PTI
[  260.231006] CPU: 1 PID: 5455 Comm: cat Kdump: loaded Tainted: G           OE K  --------- -  - 4.18.0-305.19.1.el8_4.x86_64 #1
[  260.231028] Hardware name: Virtuozzo KVM, BIOS 1.11.0-2.vz7.2 04/01/2014
[  260.231052] RIP: 0010:strlen+0x0/0x20
[  260.231061] Code: 48 89 f9 74 09 48 83 c1 01 80 39 00 75 f7 31 d2 44 0f b6 04 16 44 88 04 11 48 83 c2 01 45 84 c0 75 ee c3 0f 1f 80 00 00 00 00 <80> 3f 00 74 10 48 89 f8 48 83 c0 01 80 38 00 75 f7 48 29 f8 c3 31
[  260.231095] RSP: 0018:ffff9d9e0127fde8 EFLAGS: 00010282
[  260.231116] RAX: ffffffffc08cd311 RBX: ffff9d9e0127fe00 RCX: 0000000000000000
[  260.231130] RDX: 00007f67ac256000 RSI: 0000000000000000 RDI: ffffffffc075f60d
[  260.231144] RBP: ffffffffffffffea R08: ffff9d9e0127ff08 R09: 0000000000000000
[  260.231158] R10: ffff9d9e0127fe80 R11: 0000000000000000 R12: ffffffffaf9a53c0
[  260.231171] R13: ffff9d9e0127fe80 R14: 00007f67ac256000 R15: ffff8e2f2e5cc1c0
[  260.231185] FS:  00007f67b93f5540(0000) GS:ffff8e2f3bd00000(0000) knlGS:0000000000000000
[  260.231200] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  260.231214] CR2: ffffffffc075f60d CR3: 000000017976c004 CR4: 00000000001706e0
[  260.231230] Call Trace:
[  260.231254]  proc_sctp_do_hmac_alg+0x15b/0x190 [sctp]
[  260.231272]  proc_sys_call_handler+0x1a5/0x1c0
[  260.231287]  vfs_read+0x91/0x140
[  260.231295]  ksys_read+0x4f/0xb0
[  260.231304]  do_syscall_64+0x5b/0x1a0
[  260.231315]  entry_SYSCALL_64_after_hwframe+0x65/0xca
[  260.231327] RIP: 0033:0x7f67b931d0a5
[  260.231336] Code: fe ff ff 50 48 8d 3d f2 04 0a 00 e8 f5 fd 01 00 0f 1f 44 00 00 f3 0f 1e fa 48 8d 05 f5 5f 0d 00 8b 00 85 c0 75 0f 31 c0 0f 05 <48> 3d 00 f0 ff ff 77 53 c3 66 90 41 54 49 89 d4 55 48 89 f5 53 89
[  260.231370] RSP: 002b:00007ffda1b00468 EFLAGS: 00000246 ORIG_RAX: 0000000000000000
[  260.231835] RAX: ffffffffffffffda RBX: 0000000000020000 RCX: 00007f67b931d0a5
[  260.232289] RDX: 0000000000020000 RSI: 00007f67ac256000 RDI: 0000000000000003
[  260.232748] RBP: 00007f67ac256000 R08: 00000000ffffffff R09: 0000000000000000
[  260.233201] R10: 0000000000000022 R11: 0000000000000246 R12: 00007f67ac256000
[  260.233672] R13: 0000000000000003 R14: 0000000000000fff R15: 0000000000020000
[  260.234288] Modules linked in: sctp nft_fib_inet nft_fib_ipv4 nft_fib_ipv6 nft_fib nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct nf_tables_set nft_chain_nat ip6table_nat ip6table_mangle ip6table_raw ip6table_security iptable_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 iptable_mangle iptable_raw iptable_security ip_set nf_tables nfnetlink ip6table_filter ip6_tables iptable_filter intel_rapl_msr intel_rapl_common kvm_intel kvm snd_hda_codec_generic ledtrig_audio irqbypass snd_hda_intel crct10dif_pclmul snd_intel_dspcfg soundwire_intel qxl soundwire_generic_allocation crc32_pclmul drm_ttm_helper ttm snd_soc_core snd_compress drm_kms_helper soundwire_cadence soundwire_bus syscopyarea sysfillrect snd_hda_codec sysimgblt fb_sys_fops snd_hda_core snd_hwdep snd_seq drm snd_seq_device iTCO_wdt iTCO_vendor_support snd_pcm ghash_clmulni_intel pcspkr snd_timer snd i2c_i801 soundcore virtio_balloon lpc_ich ip_tables xfs libcrc32c sr_mod cdrom sd_mod t10_pi sg ahci libahci
[  260.234311]  libata crc32c_intel virtio_net net_failover serio_raw failover virtio_console dm_mirror dm_region_hash dm_log dm_mod [last unloaded: kpatch_sctp_str]
[  260.239265] CR2: ffffffffc075f60d

Note that ffffffffc075f60d mentioned in the "BUG" message is .rodata.proc_sctp_do_hmac_alg.str1.1+0x18, the address of "md5" string in the patch module.

As you can see, it is quite easy to miss such string references when preparing patches. They are relatively rare in the kernel but can still be found.

I guess, create-diff-object could detect that a string did not change in the patched object and replace the relocation with the one for the original data. Or, perhaps, you have better ideas how to fix it.

The text was updated successfully, but these errors were encountered:

joe-lawrence · 2021-10-06T18:39:20Z

Tough problem. As I understand it, the patch inadvertently introduced "patched" code that now sets up pointers to the patch module, pointers that may outlive the patch module lifetime :( Atomic replace doesn't help, and in fact, may only encourage more kpatch module unloading.

I don't know of any 100% perfect way to fix this problem. Substituting the relocations as suggested should solve this specific instance... but the general problem may manifest in other ways depending on the pointer type (thinking of function pointers, or did we do the same trick for those?).

Perhaps it's worth kpatch-build highlighting these changes as a non-fatal warning? Could they reasonably be checked by reviewing the changed function list vs. the input patch?

euspectre · 2021-10-07T13:05:03Z

As I understand it, the patch inadvertently introduced "patched" code that now sets up pointers to the patch module, pointers that may outlive the patch module lifetime

Exactly.

but the general problem may manifest in other ways depending on the pointer type (thinking of function pointers, or did we do the same trick for those?).

We dealt with function pointers in 495e619, to some extent. It relies on a couple heuristics, but has worked so far. Besides, processing functions is easier, because create-diff-object detects which ones are new or changed.

As for .rodata and .rodata.*str1* - if I understand it correctly, create-diff-object just takes them from the patched code as is: https://github.com/dynup/kpatch/blob/master/kpatch-build/create-diff-object.c#L1768

static void kpatch_include_standard_elements(struct kpatch_elf *kelf)
{
	struct section *sec;

	list_for_each_entry(sec, &kelf->sections, list) {
	...
			if (!strcmp(sec->name, ".shstrtab") ||
		    !strcmp(sec->name, ".strtab") ||
		    !strcmp(sec->name, ".symtab") ||
		    !strcmp(sec->name, ".toc") ||
		    !strcmp(sec->name, ".rodata") ||
		    (!strncmp(sec->name, ".rodata.", 8) &&
		     strstr(sec->name, ".str1."))) {
			kpatch_include_section(sec);
		}
	}

.rodata seems to contain only special data in the patched code, so I guess we only need to deal with .rodata.*.str1.* somehow.

euspectre · 2021-10-07T21:28:39Z

I wonder, would it help if create-diff-object used dynrelas for the references to .rodata.*.str1.*.

joe-lawrence · 2021-10-13T19:24:57Z

I wonder, would it help if create-diff-object used dynrelas for the references to .rodata.*.str1.*.

Hmm, the idea is that the dynrelas would resolve them to their originals? Are there cases in which the string contents really do need to change? And in which case, a livepatch that starting proliferating pointers to livepatch provided string contents could never be safely disabled. Maybe I'm not thinking this through, but it feels like an impossible to solve problem, for all cases anyway. Using the original relocation/contents sounds reasonable, I just wonder how to catch any instances where it's not. Those seem like inherently unsafe patches that maybe we could bring attention to somehow.

euspectre · 2021-10-14T10:34:03Z

So far, I have found only 2 places in the kernel where the problem might happen: the one in sctp I described above and also one location in the module loader code (add_sect_attrs()), where I found the issue initially:

kernel/module.c:

	/* Setup section attributes. */
	sect_attrs->grp.name = "sections";	// <<< Here
	sect_attrs->grp.attrs = (void *)sect_attrs + size[0];

	sect_attrs->nsections = 0;
	[...]
	if (sysfs_create_group(&mod->mkobj.kobj, &sect_attrs->grp))
		goto out;

	mod->sect_attrs = sect_attrs;

In both cases, it makes no sense to change the string in the patch.

Most of the time, the kernel code copies the needed strings rather than refers to them directly - it is safer this way.

…Stream The list of assignments of string pointers to non-local variables. I filtered out some records, not interesting for livepatch, e.g. the ones from lib/ and a few other places livepatch cannot handle. Note that there are still many records in the log, where such assignments are not a problem, i.e. false positives. '*error = ' and such in dm/* seem to be one common example: the assigned string seems "short-lived", it is likey only used by the callers of the given function rather than stored for later. On the other hand, the detected cases in crypto/asymmetric_keys/*.c, fs/nfs/client.c and some other might be problematic for livepatch. See dynup/kpatch#1225 for details.

euspectre · 2022-06-28T19:55:00Z

Out of curiosity, I wrote a small plugin for GCC, which reports assignment of const string pointers to non-local variables: https://github.com/euspectre/stored_strings/ .

I used it when rebuilding kernel 5.14.0-80.el9.x86_64 from CentOS 9 Stream, and it had show lots of such assignments in the kernel code. Here are the (slightly filtered) results: https://github.com/euspectre/stored_strings/blob/master/results/centos9-5.14.0-80.el9.x86_64/filtered01.log. About 1400 locations total.

It seems that many such constructs create no problems for livepatch, though.

The plugin skips '__init' functions, but there are functions with such assignments, which are not marked with '__init' but are only/mostly called from __init functions. No harm here.
Some variables the strings are assigned seem "short-lived", they are used only while the given function or its callers are running (e.g. *error = "..." in drivers/md/*). This is unlikely to cause problems too.

Unfortunately, it seems difficult to filter out such cases automatically.

However, there are many locations where livepatching might still run into the issue. Examples:

nfs_alloc_client()

fs/nfs/client.c:185:27: note: got an assignment of a const char * to a global variable.
185 | clp->cl_principal = "*";

nlmsvc_testlock()

fs/lockd/svclock.c:629:26: note: got an assignment of a const char * to a global variable.
629 | conflock->caller = "somehost"; /* FIXME */

Several functions in crypto/asymmetric_keys/*: x509_key_preparse, x509_note_pkey_algo, x509_extract_key_data, pkcs7_sig_note_digest_algo, pkcs7_sig_note_pkey_algo, mscode_note_digest_algo, pkcs8_key_preparse, pkcs8_note_algo
etc.

The GCC plugin sometimes produces false positives, so one cannot just add it to KCFLAGS in kpatch-build as is. Still, it might give hints which kernel functions might require extra care when preparing livepatches.

euspectre · 2022-06-28T20:07:48Z

As for my original ideas how to fix the issue - they won't work that easily, I'm afraid.

The problem is not because the contents of .rodata.*.str1.1 change.

kpatch_include_standard_elements() in create-diff-object always includes .rodata.*.str1.* sections, even if they are the same. So, even a livepatch that adds a "nop" in proc_sctp_do_hmac_alg() would have the issue.

proc_sctp_do_hmac_alg() from the original kernel refers to .rodata.str1.1, while the one in the patched module - to .rodata.proc_sctp_do_hmac_alg.str1.1. create-diff-object does not have access to the original .rodata.str1.1 section.

Detection of such unsafe assignments in the binary code also seems difficult: they look the same as the assignments to locals, etc.

Not sure what to do with this.
When patching kernel modules like sctp.ko, passing the original module (not the one built by kpatch-build) to create-diff-object to get the original .rodata.str1.1 can be difficult.

github-actions · 2023-08-11T10:15:36Z

This issue has been open for 30 days with no activity and no assignee. It will be closed in 7 days unless a comment is added.

github-actions · 2023-09-11T10:16:20Z

This issue has been open for 30 days with no activity and no assignee. It will be closed in 7 days unless a comment is added.

github-actions bot added the stale label Aug 11, 2023

jpoimboe removed the stale label Aug 11, 2023

github-actions bot added the stale label Sep 11, 2023

jpoimboe self-assigned this Sep 12, 2023

jpoimboe removed the stale label Sep 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kpatch-build: incorrect or missing correlation of references to .rodata.*.str1_1 leads to kernel crashes #1225

kpatch-build: incorrect or missing correlation of references to .rodata.*.str1_1 leads to kernel crashes #1225

euspectre commented Sep 24, 2021

joe-lawrence commented Oct 6, 2021

euspectre commented Oct 7, 2021

euspectre commented Oct 7, 2021

joe-lawrence commented Oct 13, 2021

euspectre commented Oct 14, 2021

euspectre commented Jun 28, 2022 •

edited

Loading

euspectre commented Jun 28, 2022 •

edited

Loading

github-actions bot commented Aug 11, 2023

github-actions bot commented Sep 11, 2023

kpatch-build: incorrect or missing correlation of references to .rodata.*.str1_1 leads to kernel crashes #1225

kpatch-build: incorrect or missing correlation of references to .rodata.*.str1_1 leads to kernel crashes #1225

Comments

euspectre commented Sep 24, 2021

joe-lawrence commented Oct 6, 2021

euspectre commented Oct 7, 2021

euspectre commented Oct 7, 2021

joe-lawrence commented Oct 13, 2021

euspectre commented Oct 14, 2021

euspectre commented Jun 28, 2022 • edited Loading

euspectre commented Jun 28, 2022 • edited Loading

github-actions bot commented Aug 11, 2023

github-actions bot commented Sep 11, 2023

euspectre commented Jun 28, 2022 •

edited

Loading

euspectre commented Jun 28, 2022 •

edited

Loading