| Message ID | 20210623134756.30930-3-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | semihosting/next (SYS_HEAPINFO) | expand |
On Wed, 23 Jun 2021 at 14:48, Alex Bennée <alex.bennee@linaro.org> wrote: > > This allows us to check our new SYS_HEAPINFO implementation generates > sane values. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > --- > tests/tcg/aarch64/system/semiheap.c | 74 +++++++++++++++++++++++++++++ > 1 file changed, 74 insertions(+) > create mode 100644 tests/tcg/aarch64/system/semiheap.c > > diff --git a/tests/tcg/aarch64/system/semiheap.c b/tests/tcg/aarch64/system/semiheap.c > new file mode 100644 > index 0000000000..d5613dca59 > --- /dev/null > +++ b/tests/tcg/aarch64/system/semiheap.c > @@ -0,0 +1,74 @@ > +/* > + * Semihosting System HEAPINFO Test > + * > + * Copyright (c) 2021 Linaro Ltd > + * > + * SPDX-License-Identifier: GPL-2.0-or-later > + */ > + > +#include <inttypes.h> > +#include <stddef.h> > +#include <minilib.h> > + > +#define SYS_HEAPINFO 0x16 > + > +uintptr_t __semi_call(uintptr_t type, uintptr_t arg0) > +{ > + register uintptr_t t asm("x0") = type; > + register uintptr_t a0 asm("x1") = arg0; > + asm("hlt 0xf000" > + : "=r" (t) > + : "r" (t), "r" (a0)); You should include "memory" in the clobbers list here, or the compiler has license to assume that the semihosting call doesn't actually write to the struct info. > + > + return t; > +} > + > +int main(int argc, char *argv[argc]) > +{ > + struct { > + void *heap_base; > + void *heap_limit; > + void *stack_base; > + void *stack_limit; > + } info; > + void *ptr_to_info = (void *) &info; > + > + ml_printf("Semihosting Heap Info Test\n"); > + > + /* memset(&info, 0, sizeof(info)); */ Why is this here but commented out ? (If you want to zero initialize the struct, using "= { }" when you define it above is simpler.) > + __semi_call(SYS_HEAPINFO, (uintptr_t) &ptr_to_info); > + > + if (info.heap_base == NULL || info.heap_limit == NULL) { > + ml_printf("null heap: %p -> %p\n", info.heap_base, info.heap_limit); > + return -1; > + } > + > + /* Error if heap base is above limit */ > + if ((uintptr_t) info.heap_base >= (uintptr_t) info.heap_limit) { > + ml_printf("heap base %p >= heap_limit %p\n", > + info.heap_base, info.heap_limit); > + return -2; > + } > + > + if (info.stack_base == NULL) { > + ml_printf("null stack: %p -> %p\n", info.stack_base, info.stack_limit); > + return -3; > + } > + > + /* > + * We don't check our local variables are inside the reported > + * stack because the runtime may select a different stack area (as > + * our boot.S code does). However we can check we don't clash with > + * the heap. > + */ > + if (ptr_to_info > info.heap_base && ptr_to_info < info.heap_limit) { > + ml_printf("info appears to be inside the heap: %p in %p:%p\n", > + ptr_to_info, info.heap_base, info.heap_limit); I'm not sure this test is valid -- the 'struct info' is on our stack, so it could be anywhere in RAM, including possibly in the big range we got back from SYS_HEAPINFO. You could if you liked check that for instance the address of 'main' is not inside the heap (assuming that you load this test case with the ELF loader, it should be in a rom blob and thus excluded from the heap range.) > + return -4; > + } > + > + ml_printf("heap: %p -> %p\n", info.heap_base, info.heap_limit); > + ml_printf("stack: %p <- %p\n", info.stack_limit, info.stack_base); > + ml_printf("Passed HeapInfo checks\n"); > + return 0; > +} It would also be useful to check that you can write to the memory and read back the value written (ie that we have not been given back a range that's read-only or which is not backed by anything). (You might need to jump through a hoop or two to check where your current stack is before potentially stomping on it...) thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Wed, 23 Jun 2021 at 14:48, Alex Bennée <alex.bennee@linaro.org> wrote: >> >> This allows us to check our new SYS_HEAPINFO implementation generates >> sane values. >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> --- >> tests/tcg/aarch64/system/semiheap.c | 74 +++++++++++++++++++++++++++++ >> 1 file changed, 74 insertions(+) >> create mode 100644 tests/tcg/aarch64/system/semiheap.c >> >> diff --git a/tests/tcg/aarch64/system/semiheap.c b/tests/tcg/aarch64/system/semiheap.c >> new file mode 100644 >> index 0000000000..d5613dca59 >> --- /dev/null >> +++ b/tests/tcg/aarch64/system/semiheap.c >> @@ -0,0 +1,74 @@ >> +/* >> + * Semihosting System HEAPINFO Test >> + * >> + * Copyright (c) 2021 Linaro Ltd >> + * >> + * SPDX-License-Identifier: GPL-2.0-or-later >> + */ >> + >> +#include <inttypes.h> >> +#include <stddef.h> >> +#include <minilib.h> >> + >> +#define SYS_HEAPINFO 0x16 >> + >> +uintptr_t __semi_call(uintptr_t type, uintptr_t arg0) >> +{ >> + register uintptr_t t asm("x0") = type; >> + register uintptr_t a0 asm("x1") = arg0; >> + asm("hlt 0xf000" >> + : "=r" (t) >> + : "r" (t), "r" (a0)); > > You should include "memory" in the clobbers list here, or the compiler > has license to assume that the semihosting call doesn't actually > write to the struct info. > >> + >> + return t; >> +} >> + >> +int main(int argc, char *argv[argc]) >> +{ >> + struct { >> + void *heap_base; >> + void *heap_limit; >> + void *stack_base; >> + void *stack_limit; >> + } info; >> + void *ptr_to_info = (void *) &info; >> + >> + ml_printf("Semihosting Heap Info Test\n"); >> + >> + /* memset(&info, 0, sizeof(info)); */ > > Why is this here but commented out ? (If you want to zero initialize > the struct, using "= { }" when you define it above is simpler.) > >> + __semi_call(SYS_HEAPINFO, (uintptr_t) &ptr_to_info); >> + >> + if (info.heap_base == NULL || info.heap_limit == NULL) { >> + ml_printf("null heap: %p -> %p\n", info.heap_base, info.heap_limit); >> + return -1; >> + } >> + >> + /* Error if heap base is above limit */ >> + if ((uintptr_t) info.heap_base >= (uintptr_t) info.heap_limit) { >> + ml_printf("heap base %p >= heap_limit %p\n", >> + info.heap_base, info.heap_limit); >> + return -2; >> + } >> + >> + if (info.stack_base == NULL) { >> + ml_printf("null stack: %p -> %p\n", info.stack_base, info.stack_limit); >> + return -3; >> + } >> + >> + /* >> + * We don't check our local variables are inside the reported >> + * stack because the runtime may select a different stack area (as >> + * our boot.S code does). However we can check we don't clash with >> + * the heap. >> + */ >> + if (ptr_to_info > info.heap_base && ptr_to_info < info.heap_limit) { >> + ml_printf("info appears to be inside the heap: %p in %p:%p\n", >> + ptr_to_info, info.heap_base, info.heap_limit); > > I'm not sure this test is valid -- the 'struct info' is on our stack, > so it could be anywhere in RAM, including possibly in the big > range we got back from SYS_HEAPINFO. It should be in this case because boot.S sets stack to be inside out data segment. > > You could if you liked check that for instance the address of 'main' > is not inside the heap (assuming that you load this test case with > the ELF loader, it should be in a rom blob and thus excluded from > the heap range.) > >> + return -4; >> + } >> + >> + ml_printf("heap: %p -> %p\n", info.heap_base, info.heap_limit); >> + ml_printf("stack: %p <- %p\n", info.stack_limit, info.stack_base); >> + ml_printf("Passed HeapInfo checks\n"); >> + return 0; >> +} > > It would also be useful to check that you can write to the memory and > read back the value written (ie that we have not been given > back a range that's read-only or which is not backed by anything). > (You might need to jump through a hoop or two to check where your > current stack is before potentially stomping on it...) > > thanks > -- PMM
On Wed, 9 Feb 2022 at 17:26, Alex Bennée <alex.bennee@linaro.org> wrote: > > > Peter Maydell <peter.maydell@linaro.org> writes: > > > On Wed, 23 Jun 2021 at 14:48, Alex Bennée <alex.bennee@linaro.org> wrote: > >> > >> This allows us to check our new SYS_HEAPINFO implementation generates > >> sane values. > >> > >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > >> --- > >> tests/tcg/aarch64/system/semiheap.c | 74 +++++++++++++++++++++++++++++ > >> 1 file changed, 74 insertions(+) > >> create mode 100644 tests/tcg/aarch64/system/semiheap.c > >> + /* > >> + * We don't check our local variables are inside the reported > >> + * stack because the runtime may select a different stack area (as > >> + * our boot.S code does). However we can check we don't clash with > >> + * the heap. > >> + */ > >> + if (ptr_to_info > info.heap_base && ptr_to_info < info.heap_limit) { > >> + ml_printf("info appears to be inside the heap: %p in %p:%p\n", > >> + ptr_to_info, info.heap_base, info.heap_limit); > > > > I'm not sure this test is valid -- the 'struct info' is on our stack, > > so it could be anywhere in RAM, including possibly in the big > > range we got back from SYS_HEAPINFO. > > It should be in this case because boot.S sets stack to be inside out > data segment. So what you mean is /* * boot.S put our stack somewhere inside the text segment of the * ELF file, and we know that SYS_HEAPINFO won't pick a range * that overlaps with part of a loaded ELF file. So the info * struct (on the stack) should not be inside the reported heap. */ ? -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Wed, 9 Feb 2022 at 17:26, Alex Bennée <alex.bennee@linaro.org> wrote: >> >> >> Peter Maydell <peter.maydell@linaro.org> writes: >> >> > On Wed, 23 Jun 2021 at 14:48, Alex Bennée <alex.bennee@linaro.org> wrote: >> >> >> >> This allows us to check our new SYS_HEAPINFO implementation generates >> >> sane values. >> >> >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> >> --- >> >> tests/tcg/aarch64/system/semiheap.c | 74 +++++++++++++++++++++++++++++ >> >> 1 file changed, 74 insertions(+) >> >> create mode 100644 tests/tcg/aarch64/system/semiheap.c >> >> + /* >> >> + * We don't check our local variables are inside the reported >> >> + * stack because the runtime may select a different stack area (as >> >> + * our boot.S code does). However we can check we don't clash with >> >> + * the heap. >> >> + */ >> >> + if (ptr_to_info > info.heap_base && ptr_to_info < info.heap_limit) { >> >> + ml_printf("info appears to be inside the heap: %p in %p:%p\n", >> >> + ptr_to_info, info.heap_base, info.heap_limit); >> > >> > I'm not sure this test is valid -- the 'struct info' is on our stack, >> > so it could be anywhere in RAM, including possibly in the big >> > range we got back from SYS_HEAPINFO. >> >> It should be in this case because boot.S sets stack to be inside out >> data segment. > > So what you mean is > > /* > * boot.S put our stack somewhere inside the text segment of the > * ELF file, and we know that SYS_HEAPINFO won't pick a range > * that overlaps with part of a loaded ELF file. So the info > * struct (on the stack) should not be inside the reported heap. > */ > > ? Well the data segment (but not the bss). So as long as the ELF loader includes that in the calculation (which it should I think) then we are ok. > > -- PMM
On Wed, 9 Feb 2022 at 18:15, Alex Bennée <alex.bennee@linaro.org> wrote: > > > Peter Maydell <peter.maydell@linaro.org> writes: > > > On Wed, 9 Feb 2022 at 17:26, Alex Bennée <alex.bennee@linaro.org> wrote: > >> It should be in this case because boot.S sets stack to be inside out > >> data segment. > > > > So what you mean is > > > > /* > > * boot.S put our stack somewhere inside the text segment of the > > * ELF file, and we know that SYS_HEAPINFO won't pick a range > > * that overlaps with part of a loaded ELF file. So the info > > * struct (on the stack) should not be inside the reported heap. > > */ > > > > ? > > Well the data segment (but not the bss). Ah, yes, I missed the ".data" when I was scanning the file. (For a system binary it doesn't matter, because our ELF loader doesn't care whether the segment is marked read-only or read-write, it just loads it into RAM.) > So as long as the ELF loader > includes that in the calculation (which it should I think) then we are > ok. Should be OK -- the ELF loader creates a rom blob for every segment in the file, and then the SYS_HEAPINFO implementation will avoid them all. -- PMM
diff --git a/tests/tcg/aarch64/system/semiheap.c b/tests/tcg/aarch64/system/semiheap.c new file mode 100644 index 0000000000..d5613dca59 --- /dev/null +++ b/tests/tcg/aarch64/system/semiheap.c @@ -0,0 +1,74 @@ +/* + * Semihosting System HEAPINFO Test + * + * Copyright (c) 2021 Linaro Ltd + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#include <inttypes.h> +#include <stddef.h> +#include <minilib.h> + +#define SYS_HEAPINFO 0x16 + +uintptr_t __semi_call(uintptr_t type, uintptr_t arg0) +{ + register uintptr_t t asm("x0") = type; + register uintptr_t a0 asm("x1") = arg0; + asm("hlt 0xf000" + : "=r" (t) + : "r" (t), "r" (a0)); + + return t; +} + +int main(int argc, char *argv[argc]) +{ + struct { + void *heap_base; + void *heap_limit; + void *stack_base; + void *stack_limit; + } info; + void *ptr_to_info = (void *) &info; + + ml_printf("Semihosting Heap Info Test\n"); + + /* memset(&info, 0, sizeof(info)); */ + __semi_call(SYS_HEAPINFO, (uintptr_t) &ptr_to_info); + + if (info.heap_base == NULL || info.heap_limit == NULL) { + ml_printf("null heap: %p -> %p\n", info.heap_base, info.heap_limit); + return -1; + } + + /* Error if heap base is above limit */ + if ((uintptr_t) info.heap_base >= (uintptr_t) info.heap_limit) { + ml_printf("heap base %p >= heap_limit %p\n", + info.heap_base, info.heap_limit); + return -2; + } + + if (info.stack_base == NULL) { + ml_printf("null stack: %p -> %p\n", info.stack_base, info.stack_limit); + return -3; + } + + /* + * We don't check our local variables are inside the reported + * stack because the runtime may select a different stack area (as + * our boot.S code does). However we can check we don't clash with + * the heap. + */ + if (ptr_to_info > info.heap_base && ptr_to_info < info.heap_limit) { + ml_printf("info appears to be inside the heap: %p in %p:%p\n", + ptr_to_info, info.heap_base, info.heap_limit); + return -4; + } + + ml_printf("heap: %p -> %p\n", info.heap_base, info.heap_limit); + ml_printf("stack: %p <- %p\n", info.stack_limit, info.stack_base); + ml_printf("Passed HeapInfo checks\n"); + return 0; +}
This allows us to check our new SYS_HEAPINFO implementation generates sane values. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- tests/tcg/aarch64/system/semiheap.c | 74 +++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) create mode 100644 tests/tcg/aarch64/system/semiheap.c -- 2.20.1