Re: [Xen-devel] [PATCH v4 03/16] arm: poison initmem when it is freed.

[Julien Grall <julien.grall@xxxxxxx>]

Hi Konrad,

On 16/09/2016 18:38, Konrad Rzeszutek Wilk wrote:
> The current byte sequence is '0xcc' which makes sense on x86,
> but on ARM it is:
>
> cccccccc        stclgt  12, cr12, [ip], {204}   ; 0xcc
>
> Picking something more ARM applicable such as:
>
> efefefef        svc     0x00efefef
>
> Creates a nice crash if one executes that code:
> (XEN) CPU1: Unexpected Trap: Supervisor Call
>
> But unfortunatly that may not be a good choice either as in the future
s/unfortunatly/unfortunately/

> we may want to implement support for it.
>
> Julien suggested that we use a 4-byte insn instruction instead
> of trying to work with one byte.
>
> As such on ARM 32 we use the udf instruction (see A8.8.247
> in ARM DDI 0406C.c) and on ARM 64 use the AARCH64_BREAK_FAULT
> instruction (aka brk instruction).
>
> We don't have to worry about Thumb code so this instruction
> is a safe to execute.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> ---
> Cc: Julien Grall <julien.grall@xxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>
> v3: New submission
> v4: Instead of using 0xef, use specific insn for architectures.
> ---
>  xen/arch/arm/mm.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 07e2037..438bed7 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -994,8 +994,23 @@ void free_init_memory(void)
>  {
>      paddr_t pa = virt_to_maddr(__init_begin);
>      unsigned long len = __init_end - __init_begin;
> +    uint32_t insn;
> +    unsigned int i, nr = len / sizeof(insn);
> +
>      set_pte_flags_on_range(__init_begin, len, mg_rw);
> -    memset(__init_begin, 0xcc, len);
> +#ifdef CONFIG_ARM_32
> +    /* udf instruction i.e (see A8.8.247 in ARM DDI 0406C.c) */
> +    insn = 0xe7f000f0;
> +#else
> +    insn = AARCH64_BREAK_FAULT;
> +#endif
> +    for ( i = 0; i < nr; i++ )
> +        *(__init_begin + i) = insn;
__init_begin is char[], so you will only copy the first byte of the 
instruction.
And because of nr = len / sizeof(insn) only 1/4 of the initmem will be 
poisoned.
So this should be something like:

uint32_t *p = (uint32_t *)__init_begin;
for ( i = 0; i < nr; i++)
   *(p + i) = insn;

> +
> +    nr = len % sizeof(insn);
> +    if ( nr )
> +        memset(__init_begin + len - nr, 0xcc, nr);
I am wondering if we should instead align __init_end to 4-byte. With a 
BUILD_BUG_ON in the code to check this assumption.
Any opinions?

> +
>      set_pte_flags_on_range(__init_begin, len, mg_clear);
>      init_domheap_pages(pa, pa + len);
>      printk("Freed %ldkB init memory.\n", (long)(__init_end-__init_begin)>>10);
Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel