Re: [Xen-devel] [PATCH v4 03/16] arm: poison initmem when it is freed.
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:
> >
> > stclgt 12, cr12, [ip], {204} ; 0xcc
> >
> > Picking something more ARM applicable such as:
> >
> > efefefefsvc 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
> > ---
> > Cc: Julien Grall
> > Cc: Stefano Stabellini
> >
> > 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
Re: [Xen-devel] [PATCH v4 03/16] arm: poison initmem when it is freed.
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:
stclgt 12, cr12, [ip], {204} ; 0xcc
Picking something more ARM applicable such as:
efefefefsvc 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
---
Cc: Julien Grall
Cc: Stefano Stabellini
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@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v4 03/16] arm: poison initmem when it is freed.
The current byte sequence is '0xcc' which makes sense on x86,
but on ARM it is:
stclgt 12, cr12, [ip], {204} ; 0xcc
Picking something more ARM applicable such as:
efefefefsvc 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
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
---
Cc: Julien Grall
Cc: Stefano Stabellini
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;
+
+nr = len % sizeof(insn);
+if ( nr )
+memset(__init_begin + len - nr, 0xcc, nr);
+
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);
--
2.5.5
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
