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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Mon, Sep 19, 2016 at 11:35:57AM +0200, Julien Grall wrote:
> 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@oracle.com>
> > ---
> > Cc: Julien Grall <julien.grall@arm.com>
> > Cc: Stefano Stabellini <sstabellini@kernel.org>
> > 
> > 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;
> 

Yes of course!
> > +
> > +    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.

The __init_end is already aligned:

175   . = ALIGN(STACK_SIZE);                                                        
176   __init_end = .;            

So we are good there, but I do like the BUILD_BUG_ON. Let me do that.
> 
> 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@lists.xen.org
https://lists.xen.org/xen-devel