From 70ecbbcc86df7bcd61d666006f28cb91c129ccbe Mon Sep 17 00:00:00 2001 From: Nicolas Viennot Date: Fri, 17 Jul 2020 02:13:35 +0000 Subject: [PATCH] compel: allocate the GOT table to avoid memory corruption Previously, the GOT table was using the same memory location as the args region, leading to difficult to debug memory corruption bugs. We allocate the GOT table between the parasite blob and the args region. The reason this is a good placement is: 1) Putting it after the args region is possible but a bit combersome as the args region has a variable size 2) The cr-restore.c code maps the parasite code without the args region, as it does not do RPC. Another option is to rely on the linker to generate a GOT section, but I failed to do so despite my best attempts. Signed-off-by: Nicolas Viennot --- compel/include/uapi/infect.h | 2 +- compel/src/lib/handle-elf.c | 5 ++--- compel/src/lib/infect.c | 21 ++++++++++++++++----- criu/cr-restore.c | 23 +++++++++++++++++++---- criu/parasite-syscall.c | 1 - criu/pie/pie-relocs.h | 10 ---------- 6 files changed, 38 insertions(+), 24 deletions(-) delete mode 100644 criu/pie/pie-relocs.h diff --git a/compel/include/uapi/infect.h b/compel/include/uapi/infect.h index 8f9de0e1c..257658a70 100644 --- a/compel/include/uapi/infect.h +++ b/compel/include/uapi/infect.h @@ -150,10 +150,10 @@ struct parasite_blob_desc { struct { const void *mem; size_t bsize; - size_t nr_gotpcrel; unsigned long parasite_ip_off; unsigned long cmd_off; unsigned long args_ptr_off; + unsigned long got_off; unsigned long args_off; compel_reloc_t *relocs; unsigned int nr_relocs; diff --git a/compel/src/lib/handle-elf.c b/compel/src/lib/handle-elf.c index 8a79151b4..9646dd4dc 100644 --- a/compel/src/lib/handle-elf.c +++ b/compel/src/lib/handle-elf.c @@ -648,7 +648,6 @@ int __handle_elf(void *mem, size_t size) } #endif /* !NO_RELOCS */ pr_out("};\n"); - pr_out("static __maybe_unused size_t %s_nr_gotpcrel = %zd;\n", opts.prefix, nr_gotpcrel); pr_out("static __maybe_unused const char %s_blob[] = {\n\t", opts.prefix); @@ -690,7 +689,6 @@ int __handle_elf(void *mem, size_t size) pr_out("\tpbd->hdr.mem = %s_blob;\n", opts.prefix); pr_out("\tpbd->hdr.bsize = sizeof(%s_blob);\n", opts.prefix); - pr_out("\tpbd->hdr.nr_gotpcrel = %s_nr_gotpcrel;\n", opts.prefix); pr_out("\tif (native)\n"); pr_out("\t\tpbd->hdr.parasite_ip_off = " "%s_sym__export_parasite_head_start;\n", opts.prefix); @@ -701,7 +699,8 @@ int __handle_elf(void *mem, size_t size) pr_out("#endif /* CONFIG_COMPAT */\n"); pr_out("\tpbd->hdr.cmd_off = %s_sym__export_parasite_service_cmd;\n", opts.prefix); pr_out("\tpbd->hdr.args_ptr_off = %s_sym__export_parasite_service_args_ptr;\n", opts.prefix); - pr_out("\tpbd->hdr.args_off = round_up(pbd->hdr.bsize, 4);\n"); + pr_out("\tpbd->hdr.got_off = round_up(pbd->hdr.bsize, sizeof(long));\n"); + pr_out("\tpbd->hdr.args_off = pbd->hdr.got_off + %zd*sizeof(long);\n", nr_gotpcrel); pr_out("\tpbd->hdr.relocs = %s_relocs;\n", opts.prefix); pr_out("\tpbd->hdr.nr_relocs = " "sizeof(%s_relocs) / sizeof(%s_relocs[0]);\n", diff --git a/compel/src/lib/infect.c b/compel/src/lib/infect.c index da02d1744..a48d5ff7d 100644 --- a/compel/src/lib/infect.c +++ b/compel/src/lib/infect.c @@ -816,11 +816,11 @@ err_cure: void compel_relocs_apply(void *mem, void *vbase, struct parasite_blob_desc *pbd) { - size_t size = pbd->hdr.bsize; compel_reloc_t *elf_relocs = pbd->hdr.relocs; size_t nr_relocs = pbd->hdr.nr_relocs; size_t i, j; + void **got = mem + pbd->hdr.got_off; /* * parasite_service() reads the value of __export_parasite_service_args_ptr. @@ -835,14 +835,13 @@ void compel_relocs_apply(void *mem, void *vbase, struct parasite_blob_desc *pbd) for (i = 0, j = 0; i < nr_relocs; i++) { if (elf_relocs[i].type & COMPEL_TYPE_LONG) { long *where = mem + elf_relocs[i].offset; - long *p = mem + size; if (elf_relocs[i].type & COMPEL_TYPE_GOTPCREL) { int *value = (int *)where; int rel; - p[j] = (long)vbase + elf_relocs[i].value; - rel = (unsigned)((void *)&p[j] - (void *)mem) - elf_relocs[i].offset + elf_relocs[i].addend; + got[j] = vbase + elf_relocs[i].value; + rel = (unsigned)((void *)&got[j] - (void *)mem) - elf_relocs[i].offset + elf_relocs[i].addend; *value = rel; j++; @@ -899,7 +898,9 @@ int compel_infect(struct parasite_ctl *ctl, unsigned long nr_threads, unsigned l * +------------------------------------------------------+ <--- 0 * | Parasite blob (sizeof(parasite_blob)) | * +------------------------------------------------------+ <--- hdr.bsize - * align 4 + * align 8 + * +------------------------------------------------------+ <--- hdr.got_off + * | GOT Table (nr_gotpcrel * sizeof(long)) | * +------------------------------------------------------+ <--- hdr.args_off * | Args area (args_size) | * +------------------------------------------------------+ @@ -936,6 +937,16 @@ int compel_infect(struct parasite_ctl *ctl, unsigned long nr_threads, unsigned l ctl->cmd = ctl->local_map + ctl->pblob.hdr.cmd_off; ctl->args = ctl->local_map + ctl->pblob.hdr.args_off; + /* + * args must be 4 bytes aligned as we use futexes() on them. It is + * already the case, as args follows the GOT table, which is 8 bytes + * aligned. + */ + if ((unsigned long)ctl->args & (4-1)) { + pr_err("BUG: args are not 4 bytes aligned: %p\n", ctl->args); + ctl->args = (void *)round_up((unsigned long)ctl->args, 4); + } + memcpy(ctl->local_map, ctl->pblob.hdr.mem, ctl->pblob.hdr.bsize); compel_relocs_apply(ctl->local_map, ctl->remote_map, &ctl->pblob); diff --git a/criu/cr-restore.c b/criu/cr-restore.c index 3d135af4b..edb30af20 100644 --- a/criu/cr-restore.c +++ b/criu/cr-restore.c @@ -96,8 +96,6 @@ #include "cr-errno.h" -#include "pie/pie-relocs.h" - #ifndef arch_export_restore_thread #define arch_export_restore_thread __export_restore_thread #endif @@ -2913,7 +2911,23 @@ static int prepare_restorer_blob(void) * in turn will lead to set-exe-file prctl to fail with EBUSY. */ - restorer_len = pie_size(restorer); + struct parasite_blob_desc pbd; + + /* + * We pass native=true, which is then used to set the value of + * pbd.parasite_ip_off. We don't use parasite_ip_off, so the value we + * pass as native argument is not relevant. + */ + restorer_setup_c_header_desc(&pbd, true); + + /* + * args_off is the offset where the binary blob with its GOT table + * ends. As we don't do RPC, parasite sections after args_off can be + * ignored. See compel_infect() for a description of the parasite + * memory layout. + */ + restorer_len = round_up(pbd.hdr.args_off, page_size()); + restorer = mmap(NULL, restorer_len, PROT_READ | PROT_WRITE | PROT_EXEC, MAP_PRIVATE | MAP_ANONYMOUS, 0, 0); @@ -2922,7 +2936,8 @@ static int prepare_restorer_blob(void) return -1; } - memcpy(restorer, &restorer_blob, sizeof(restorer_blob)); + memcpy(restorer, pbd.hdr.mem, pbd.hdr.bsize); + return 0; } diff --git a/criu/parasite-syscall.c b/criu/parasite-syscall.c index 5f9de152a..c7074c7c4 100644 --- a/criu/parasite-syscall.c +++ b/criu/parasite-syscall.c @@ -39,7 +39,6 @@ #include "dump.h" #include "restorer.h" -#include "pie/pie-relocs.h" #include "infect.h" #include "infect-rpc.h" diff --git a/criu/pie/pie-relocs.h b/criu/pie/pie-relocs.h deleted file mode 100644 index e36126be6..000000000 --- a/criu/pie/pie-relocs.h +++ /dev/null @@ -1,10 +0,0 @@ -#ifndef __PIE_RELOCS_H__ -#define __PIE_RELOCS_H__ - -#include "common/config.h" -#include "common/compiler.h" - -#define pie_size(__pie_name) (round_up(sizeof(__pie_name##_blob) + \ - __pie_name ## _nr_gotpcrel * sizeof(long), page_size())) - -#endif /* __PIE_RELOCS_H__ */