[CRIU] [RFC] compel: mprotect() parasite's .text from writes
Laurent Dufour
ldufour at linux.vnet.ibm.com
Mon Jan 30 02:41:02 PST 2017
On 27/01/2017 17:35, Dmitry Safonov wrote:
> Make parasite's .text section rx and data section rw.
> That'll make parasite more resistant to corruptions.
>
> For RFC sending version that always protects parasite,
> the overhead is added by mprotect() syscall.
> I can make a new infect-flag if this is too much for
> the default case.
I do like the idea but test fails on ppc64le :
======================= Run zdtm/static/dumpable01 in h
========================
Start test
./dumpable01 --pidfile=dumpable01.pid --outfile=dumpable01.out
Run criu dump
=[log]=> dump/zdtm/static/dumpable01/15/1/dump.log
------------------------ grep Error ------------------------
(00.006646) Error (criu/parasite-syscall.c:93): si_code=4 si_pid=15
si_status=11
(00.006650) Error (criu/parasite-syscall.c:101): 15 was stopped by 11
unexpectedly
------------------------ ERROR OVER ------------------------
################ Test zdtm/static/dumpable01 FAIL at CRIU dump
#################
I didn't take much time looking deeper why it fails.
Unfortunately I don't have free couples at the moment to look at this :(
Cheers,
Laurent.
>
> Cc: Cyrill Gorcunov <gorcunov at openvz.org>
> Cc: Laurent Dufour <ldufour at linux.vnet.ibm.com>
> Signed-off-by: Dmitry Safonov <dsafonov at virtuozzo.com>
> ---
> compel/arch/aarch64/plugins/std/parasite-head.S | 6 ++++--
> compel/arch/aarch64/scripts/compel-pack.lds.S | 23 +++++++++++++-------
> compel/arch/arm/plugins/std/parasite-head.S | 7 ++++---
> compel/arch/arm/scripts/compel-pack.lds.S | 23 +++++++++++++-------
> compel/arch/ppc64/plugins/std/parasite-head.S | 7 ++++---
> compel/arch/ppc64/scripts/compel-pack.lds.S | 21 +++++++++++--------
> compel/arch/x86/plugins/std/parasite-head.S | 1 +
> compel/arch/x86/scripts/compel-pack.lds.S | 22 +++++++++++--------
> compel/src/lib/handle-elf.c | 28 ++++++++++++++++++-------
> compel/src/lib/infect.c | 28 +++++++++++++++++++++++--
> 10 files changed, 114 insertions(+), 52 deletions(-)
>
> diff --git a/compel/arch/aarch64/plugins/std/parasite-head.S b/compel/arch/aarch64/plugins/std/parasite-head.S
> index 5e7067f6ba9e..09ec6db264bc 100644
> --- a/compel/arch/aarch64/plugins/std/parasite-head.S
> +++ b/compel/arch/aarch64/plugins/std/parasite-head.S
> @@ -15,6 +15,8 @@ ENTRY(__export_parasite_head_start)
> parasite_args_ptr:
> .quad __export_parasite_args
>
> -__export_parasite_cmd:
> - .quad 0
> END(__export_parasite_head_start)
> +
> + .section .compel.data, "aw"
> +GLOBAL(__export_parasite_cmd)
> + .quad 0
> diff --git a/compel/arch/aarch64/scripts/compel-pack.lds.S b/compel/arch/aarch64/scripts/compel-pack.lds.S
> index b07e68c1d905..ba5d9a75c674 100644
> --- a/compel/arch/aarch64/scripts/compel-pack.lds.S
> +++ b/compel/arch/aarch64/scripts/compel-pack.lds.S
> @@ -5,18 +5,29 @@ SECTIONS
> *(.head.text)
> *(.text*)
> . = ALIGN(32);
> - *(.data*)
> - . = ALIGN(32);
> *(.rodata*)
> . = ALIGN(32);
> - *(.bss*)
> - . = ALIGN(32);
> *(.got*)
> . = ALIGN(32);
> *(.toc*)
> . = ALIGN(32);
> } =0x00000000,
>
> + /* RW data starts on parasite_cmd, check compel_protect_parasite() */
> + . = ALIGN(0x1000);
> +
> + .data : {
> + *(.compel.data) /* it's parasite_cmd */
> + . = ALIGN(32);
> + *(.data*)
> + . = ALIGN(32);
> + *(.bss*)
> + }
> +
> + /* Parasite args should have 4 bytes align, as we have futex inside. */
> + . = ALIGN(4);
> + __export_parasite_args = .;
> +
> /DISCARD/ : {
> *(.debug*)
> *(.comment*)
> @@ -25,8 +36,4 @@ SECTIONS
> *(.eh_frame*)
> *(*)
> }
> -
> -/* Parasite args should have 4 bytes align, as we have futex inside. */
> -. = ALIGN(4);
> -__export_parasite_args = .;
> }
> diff --git a/compel/arch/arm/plugins/std/parasite-head.S b/compel/arch/arm/plugins/std/parasite-head.S
> index e72646b50804..12c6239afd27 100644
> --- a/compel/arch/arm/plugins/std/parasite-head.S
> +++ b/compel/arch/arm/plugins/std/parasite-head.S
> @@ -4,7 +4,7 @@
> ENTRY(__export_parasite_head_start)
> sub r2, pc, #8 @ get the address of this instruction
>
> - adr r0, __export_parasite_cmd
> + ldr r0, =__export_parasite_cmd
> ldr r0, [r0]
>
> adr r1, parasite_args_ptr
> @@ -16,7 +16,8 @@ ENTRY(__export_parasite_head_start)
>
> parasite_args_ptr:
> .long __export_parasite_args
> +END(__export_parasite_head_start)
>
> -__export_parasite_cmd:
> + .section .compel.data, "aw"
> +GLOBAL(__export_parasite_cmd)
> .long 0
> -END(__export_parasite_head_start)
> diff --git a/compel/arch/arm/scripts/compel-pack.lds.S b/compel/arch/arm/scripts/compel-pack.lds.S
> index fe402787f6fc..14ba5cb0afc3 100644
> --- a/compel/arch/arm/scripts/compel-pack.lds.S
> +++ b/compel/arch/arm/scripts/compel-pack.lds.S
> @@ -5,18 +5,29 @@ SECTIONS
> *(.head.text)
> *(.text*)
> . = ALIGN(32);
> - *(.data*)
> - . = ALIGN(32);
> *(.rodata*)
> . = ALIGN(32);
> - *(.bss*)
> - . = ALIGN(32);
> *(.got*)
> . = ALIGN(32);
> *(.toc*)
> . = ALIGN(32);
> } =0x00000000,
>
> + /* RW data starts on parasite_cmd, check compel_protect_parasite() */
> + . = ALIGN(0x1000);
> +
> + .data : {
> + *(.compel.data) /* it's parasite_cmd */
> + . = ALIGN(32);
> + *(.data*)
> + . = ALIGN(32);
> + *(.bss*)
> + }
> +
> + /* Parasite args should have 4 bytes align, as we have futex inside. */
> + . = ALIGN(4);
> + __export_parasite_args = .;
> +
> /DISCARD/ : {
> *(.debug*)
> *(.comment*)
> @@ -25,8 +36,4 @@ SECTIONS
> *(.eh_frame*)
> *(*)
> }
> -
> -/* Parasite args should have 4 bytes align, as we have futex inside. */
> -. = ALIGN(4);
> -__export_parasite_args = .;
> }
> diff --git a/compel/arch/ppc64/plugins/std/parasite-head.S b/compel/arch/ppc64/plugins/std/parasite-head.S
> index c870efdc209d..23d16f0350d9 100644
> --- a/compel/arch/ppc64/plugins/std/parasite-head.S
> +++ b/compel/arch/ppc64/plugins/std/parasite-head.S
> @@ -39,7 +39,8 @@ parasite_service_ptr:
> // FIXME: There should be a way to specify the global entry here.
> .quad parasite_service - 8
>
> -__export_parasite_cmd:
> - .long 0
> -
> END(__export_parasite_head_start)
> +
> + .section .compel.data, "aw"
> +GLOBAL(__export_parasite_cmd)
> + .long 0
> diff --git a/compel/arch/ppc64/scripts/compel-pack.lds.S b/compel/arch/ppc64/scripts/compel-pack.lds.S
> index 5da9a08460c6..f1abcec4f33e 100644
> --- a/compel/arch/ppc64/scripts/compel-pack.lds.S
> +++ b/compel/arch/ppc64/scripts/compel-pack.lds.S
> @@ -8,11 +8,6 @@ SECTIONS
> *(.compel.init)
> }
>
> - .data : {
> - *(.data*)
> - *(.bss*)
> - }
> -
> .rodata : {
> *(.rodata*)
> *(.got*)
> @@ -22,6 +17,18 @@ SECTIONS
> *(.toc*)
> }
>
> + /* RW data starts on parasite_cmd, check compel_protect_parasite() */
> + . = ALIGN(0x1000);
> + .data : {
> + *(.compel.data) /* it's parasite_cmd */
> + *(.data*)
> + *(.bss*)
> + }
> +
> + /* Parasite args should have 4 bytes align, as we have futex inside. */
> + . = ALIGN(4);
> + __export_parasite_args = .;
> +
> /DISCARD/ : {
> *(.debug*)
> *(.comment*)
> @@ -29,8 +36,4 @@ SECTIONS
> *(.group*)
> *(.eh_frame*)
> }
> -
> -/* Parasite args should have 4 bytes align, as we have futex inside. */
> -. = ALIGN(4);
> -__export_parasite_args = .;
> }
> diff --git a/compel/arch/x86/plugins/std/parasite-head.S b/compel/arch/x86/plugins/std/parasite-head.S
> index a988de9d4218..df63831d7f02 100644
> --- a/compel/arch/x86/plugins/std/parasite-head.S
> +++ b/compel/arch/x86/plugins/std/parasite-head.S
> @@ -47,6 +47,7 @@ ENTRY(__export_parasite_head_start)
> int $0x03
> END(__export_parasite_head_start)
>
> + .section .compel.data, "aw"
> .align 8
> GLOBAL(__export_parasite_cmd)
> .long 0
> diff --git a/compel/arch/x86/scripts/compel-pack.lds.S b/compel/arch/x86/scripts/compel-pack.lds.S
> index cd6584ffe710..da82e595afdc 100644
> --- a/compel/arch/x86/scripts/compel-pack.lds.S
> +++ b/compel/arch/x86/scripts/compel-pack.lds.S
> @@ -9,11 +9,6 @@ SECTIONS
> *(.compel.init)
> }
>
> - .data : {
> - *(.data*)
> - *(.bss*)
> - }
> -
> .rodata : {
> *(.rodata*)
> *(.got*)
> @@ -23,6 +18,19 @@ SECTIONS
> *(.toc*)
> }
>
> + /* RW data starts on parasite_cmd, check compel_protect_parasite() */
> + . = ALIGN(0x1000);
> +
> + .data : {
> + *(.compel.data) /* it's parasite_cmd */
> + *(.data*)
> + *(.bss*)
> + }
> +
> + /* Parasite args should have 4 bytes align, as we have futex inside. */
> + . = ALIGN(4);
> + __export_parasite_args = .;
> +
> /DISCARD/ : {
> *(.debug*)
> *(.comment*)
> @@ -30,8 +38,4 @@ SECTIONS
> *(.group*)
> *(.eh_frame*)
> }
> -
> -/* Parasite args should have 4 bytes align, as we have futex inside. */
> -. = ALIGN(4);
> -__export_parasite_args = .;
> }
> diff --git a/compel/src/lib/handle-elf.c b/compel/src/lib/handle-elf.c
> index 4b8b73dfacb4..e4692f71e5d0 100644
> --- a/compel/src/lib/handle-elf.c
> +++ b/compel/src/lib/handle-elf.c
> @@ -459,29 +459,41 @@ int __handle_elf(void *mem, size_t size)
> goto err;
> break;
>
> - case R_PPC64_REL16_HA:
> + case R_PPC64_REL16_HA: {
> + uint16_t inst = (*(uint32_t*)where) >> 16;
> +
> value64 += addend64 - place;
> pr_debug("\t\t\tR_PPC64_REL16_HA at 0x%-4lx val 0x%lx\n",
> place, value64);
> - /* check that we are dealing with the addis 2,12 instruction */
> - if (((*(uint32_t*)where) & 0xffff0000) != 0x3c4c0000) {
> - pr_err("Unexpected instruction for R_PPC64_REL16_HA\n");
> + /*
> + * Check that we are dealing with the addis 2,12 instruction
> + * or `addis r3,r2`
> + */
> + if (inst != 0x3c4c && inst != 0x3c62) {
> + pr_err("Unexpected instruction %#x for R_PPC64_REL16_HA\n", (unsigned)inst);
> goto err;
> }
> *(uint16_t *)where = ((value64 + 0x8000) >> 16) & 0xffff;
> break;
> + }
> +
> + case R_PPC64_REL16_LO: {
> + uint16_t inst = (*(uint32_t*)where) >> 16;
>
> - case R_PPC64_REL16_LO:
> value64 += addend64 - place;
> pr_debug("\t\t\tR_PPC64_REL16_LO at 0x%-4lx val 0x%lx\n",
> place, value64);
> - /* check that we are dealing with the addi 2,2 instruction */
> - if (((*(uint32_t*)where) & 0xffff0000) != 0x38420000) {
> - pr_err("Unexpected instruction for R_PPC64_REL16_LO\n");
> + /*
> + * Check that we are dealing with the addi 2,2 instruction
> + * or `addi r3,r2`
> + */
> + if (inst != 0x3842 && inst != 0x3862) {
> + pr_err("Unexpected instruction %#x for R_PPC64_REL16_LO\n", (unsigned)inst);
> goto err;
> }
> *(uint16_t *)where = value64 & 0xffff;
> break;
> + }
>
> #endif /* ELF_PPC64 */
>
> diff --git a/compel/src/lib/infect.c b/compel/src/lib/infect.c
> index 599c37274c6b..cd8d88231121 100644
> --- a/compel/src/lib/infect.c
> +++ b/compel/src/lib/infect.c
> @@ -703,7 +703,7 @@ static int parasite_mmap_exchange(struct parasite_ctl *ctl, unsigned long size)
> int fd;
>
> ctl->remote_map = remote_mmap(ctl, NULL, size,
> - PROT_READ | PROT_WRITE | PROT_EXEC,
> + PROT_READ | PROT_WRITE,
> MAP_ANONYMOUS | MAP_SHARED, -1, 0);
> if (!ctl->remote_map) {
> pr_err("Can't allocate memory for parasite blob (pid: %d)\n", ctl->rpid);
> @@ -781,7 +781,7 @@ static int parasite_memfd_exchange(struct parasite_ctl *ctl, unsigned long size)
> }
>
> ctl->remote_map = remote_mmap(ctl, NULL, size,
> - PROT_READ | PROT_WRITE | PROT_EXEC,
> + PROT_READ | PROT_WRITE,
> MAP_FILE | MAP_SHARED, fd, 0);
> if (!ctl->remote_map) {
> pr_err("Can't rmap memfd for parasite blob\n");
> @@ -854,6 +854,25 @@ static inline unsigned long total_pie_size(size_t blob_size, size_t nr_gp)
> return round_up(blob_size + nr_gp * sizeof(long), page_size());
> }
>
> +static int compel_protect_parasite(struct parasite_ctl *ctl)
> +{
> + bool __maybe_unused compat_task = !compel_mode_native(ctl);
> + /* RW parasite area starts from parasite_cmd */
> + uintptr_t ro_size = ctl->pblob.hdr.addr_cmd_off;
> + long ret;
> +
> + compel_syscall(ctl, __NR(mprotect, compat_task), &ret,
> + (unsigned long)ctl->remote_map, ro_size,
> + PROT_READ | PROT_EXEC, 0, 0, 0);
> + if (ret) {
> + pr_err("mprotect() parasites [%#lx, %#lx) failed with %ld\n",
> + (unsigned long)ctl->remote_map,
> + (unsigned long)ctl->remote_map + ro_size, ret);
> + return -1;
> + }
> + return 0;
> +}
> +
> int compel_infect(struct parasite_ctl *ctl, unsigned long nr_threads, unsigned long args_size)
> {
> int ret;
> @@ -914,6 +933,11 @@ int compel_infect(struct parasite_ctl *ctl, unsigned long nr_threads, unsigned l
> ctl->r_thread_stack = ctl->remote_map + p;
> }
>
> + pr_info("Protecting parasite's text from writes\n");
> + if (compel_protect_parasite(ctl))
> + goto err;
> +
> + pr_info("Starting parasite daemon\n");
> if (parasite_start_daemon(ctl))
> goto err;
>
More information about the CRIU
mailing list