[PATCH v4 1/3] Strict thread-stack isolation
Gedare Bloom
gedare at rtems.org
Sat Aug 22 18:01:51 UTC 2020
I have some comments below. I'm not that happy with the lack of design
discussion during the iteration of this code. While it is a little
easier to critique the code, it is also a bit wasteful because I have
to also comment on stylistic problems, and when some decisions you
make while coding end up not being correct or aren't easily understood
during code review then you might have to spend more time recoding. It
can be hard to find the balance between design and code reviews,
On Sat, Aug 22, 2020 at 9:19 AM Utkarsh Rai <utkarsh.rai60 at gmail.com> wrote:
>
> - These patches are based on the new build system.
> - Allocation of page aligned stack has been done through posix_memalign(),
> which is a hack.
> ---
> .../realview-pbx-a9/mmu/bsp-set-mmu-attr.c | 90 +++++++++++++
> bsps/shared/start/stackalloc.c | 12 +-
> cpukit/include/rtems/score/memoryprotection.h | 83 ++++++++++++
> cpukit/include/rtems/score/stack.h | 56 ++++++++
> cpukit/include/rtems/score/stackprotection.h | 80 +++++++++++
> cpukit/posix/src/shmopen.c | 61 +++++++++
> cpukit/score/cpu/arm/cpu_asm.S | 94 +++++++++++++
> .../score/cpu/arm/include/rtems/score/cpu.h | 2 +-
> cpukit/score/src/stackprotection.c | 125 ++++++++++++++++++
> cpukit/score/src/threadhandler.c | 6 +
> .../samples/thread_stack_protection/init.c | 114 ++++++++++++++++
> .../thread_stack_protection.doc | 2 +
> .../thread_stack_protection.scn | 20 +++
> 13 files changed, 743 insertions(+), 2 deletions(-)
> create mode 100644 bsps/arm/realview-pbx-a9/mmu/bsp-set-mmu-attr.c
> create mode 100644 cpukit/include/rtems/score/memoryprotection.h
> create mode 100644 cpukit/include/rtems/score/stackprotection.h
> create mode 100644 cpukit/score/src/stackprotection.c
> create mode 100644 testsuites/samples/thread_stack_protection/init.c
> create mode 100644 testsuites/samples/thread_stack_protection/thread_stack_protection.doc
> create mode 100644 testsuites/samples/thread_stack_protection/thread_stack_protection.scn
>
> diff --git a/bsps/arm/realview-pbx-a9/mmu/bsp-set-mmu-attr.c b/bsps/arm/realview-pbx-a9/mmu/bsp-set-mmu-attr.c
> new file mode 100644
> index 0000000000..4fba1e2b22
> --- /dev/null
> +++ b/bsps/arm/realview-pbx-a9/mmu/bsp-set-mmu-attr.c
> @@ -0,0 +1,90 @@
> +#include <bsp/arm-cp15-start.h>
> +#include <rtems/score/memoryprotection.h>
> +#include <rtems/score/thread.h>
> +#include <libcpu/arm-cp15.h>
> +#include <rtems.h>
> +
> +static uint32_t translate_flags(uint32_t attr_flags)
> +{
> + uint32_t flags;
> + uint32_t memory_attribute;
> +
> + if( ( attr_flags & RTEMS_READ_WRITE ) == RTEMS_READ_WRITE ) {
space after if
> + flags |= ARMV7_MMU_READ_WRITE;
> + }
> + if ( ( attr_flags & RTEMS_READ_ONLY ) == RTEMS_READ_ONLY ) {
> + flags |= ARMV7_MMU_READ_ONLY;
> + }
> + if ( (attr_flags & RTEMS_NO_ACCESS ) == RTEMS_NO_ACCESS ) {
RTEMS_NO_ACCESS is 0. This is always true?
> + flags = 0;
> + }
> + /*
> + * Check for memory-cache operation
> + */
> + if( attr_flags & RTEMS_MEMORY_CACHED ) {
> + flags |= ARM_MMU_SECT_TEX_0 | ARM_MMU_SECT_C | ARM_MMU_SECT_B;
> + }
> +
> + return flags;
> +}
> +
> +static rtems_status_code validate_memory( uintptr_t begin, uintptr_t end )
> +{
> + /* We can set/unset the memory attributes of the regions between the bsp stack
> + * section and the workspace section only.
> + */
> + if( begin < bsp_section_stack_begin || end > bsp_section_work_end ) {
ws
> + return RTEMS_INVALID_ADDRESS;
> + } else {
> + return RTEMS_SUCCESSFUL;
> + }
> +
> +}
> +
> +rtems_status_code _Memory_protection_Set_entries(uintptr_t begin, size_t size, uint32_t flags)
> +{
> + uintptr_t end;
> + rtems_interrupt_level irq_level;
> + uint32_t access_flags;
> +
> + end = begin + size;
> + if( validate_memory( begin, end ) == RTEMS_SUCCESSFUL ) {
ws
you should also validate the flags requested for the memory region
> + access_flags = translate_flags(flags);
ws within function call parameter list, fix throughout
> +
> + /*
> + * The ARM reference manual instructs to disable all the interrupts before
> + * setting up page table entries.
> + */
> + rtems_interrupt_disable(irq_level);
> + arm_cp15_set_translation_table_entries(begin, end, access_flags);
> + rtems_interrupt_enable(irq_level);
> +
> + return RTEMS_SUCCESSFUL;
> + }
> +
> + return RTEMS_INVALID_ADDRESS;
> +}
> +
> +rtems_status_code _Memory_protection_Unset_entries(uintptr_t begin, size_t size)
> +{
> + uint32_t access_flags;
> + uintptr_t end;
> + rtems_interrupt_level irq_level;
> +
> + if( validate_memory( begin, end ) == RTEMS_SUCCESSFUL ) {
> + end = begin + size;
> + access_flags = translate_flags( RTEMS_READ_ONLY );
Shouldn't this be NONE?
> +
> + /*
> + * The ARM reference manual instructs to disable all the interrupts before
> + * setting up page table entries.
> + */
> + rtems_interrupt_disable(irq_level);
> + arm_cp15_set_translation_table_entries(begin, end, access_flags);
> + rtems_interrupt_enable(irq_level);
> +
> + return RTEMS_SUCCESSFUL;
> + }
> +
> + return RTEMS_INVALID_ADDRESS;
> +}
> \ No newline at end of file
> diff --git a/bsps/shared/start/stackalloc.c b/bsps/shared/start/stackalloc.c
> index f7cf7be0f1..bf43e51198 100644
> --- a/bsps/shared/start/stackalloc.c
> +++ b/bsps/shared/start/stackalloc.c
> @@ -25,6 +25,7 @@
> #include <rtems.h>
> #include <rtems/score/heapimpl.h>
> #include <rtems/score/wkspace.h>
> +#include <rtems/score/memoryprotection.h>
>
> #include <bsp/linker-symbols.h>
>
> @@ -43,7 +44,15 @@ void bsp_stack_allocate_init(size_t stack_space_size)
> void *bsp_stack_allocate(size_t size)
> {
> void *stack = NULL;
> -
> +#if defined (RTEMS_THREAD_STACK_PROTECTION)
> +/*
> + * This is a temporary hack, we need to use _Heap_Allocate_aligned() but the heap
> + * initialization for bsp_stack_heap fails as bsp_section_stack_size is 0. See
> + * bsp_stack_allocate_init().
> + */
> + posix_memalign(&stack, 4096 , size);
> + _Memory_protection_Set_entries( stack, size, ( RTEMS_READ_ONLY | RTEMS_MEMORY_CACHED ) );
> +#else
> if (bsp_stack_heap.area_begin != 0) {
> stack = _Heap_Allocate(&bsp_stack_heap, size);
> }
> @@ -51,6 +60,7 @@ void *bsp_stack_allocate(size_t size)
> if (stack == NULL) {
> stack = _Workspace_Allocate(size);
> }
> +#endif
>
> return stack;
> }
> diff --git a/cpukit/include/rtems/score/memoryprotection.h b/cpukit/include/rtems/score/memoryprotection.h
> new file mode 100644
> index 0000000000..be4ce20b22
> --- /dev/null
> +++ b/cpukit/include/rtems/score/memoryprotection.h
> @@ -0,0 +1,83 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +
> +/**
> + * @file
> + *
> + * @ingroup RTEMSScoreMemoryprotection
> + *
> + * @brief This file provodes APIs for high-level memory protection
typo
I noted this typo last time.
> + */
> +
> +/*
> + * Copyright (C) 2020 Utkarsh Rai
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> + * POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#ifndef _RTEMS_SCORE_MEMORYPROTECTION_H
> +#define _RTEMS_SCORE_MEMORYPROTECTION_H
> +
> +#include <rtems/score/basedefs.h>
> +#include <rtems.h>
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#define RTEMS_NO_ACCESS 0x00
> +#define RTEMS_READ_ONLY 0x01
> +#define RTEMS_WRITE_ONLY 0x02
Maybe remove "ONLY" since they can be combined?
> +#define RTEMS_READ_WRITE ( RTEMS_READ_ONLY | RTEMS_WRITE_ONLY )
> +#define RTEMS_MEMORY_CACHED 0x04
> +
> +/**
> + * @brief Define the memory access permission for the specified memory region
> + *
> + * @param begin_addr Beginning of the memory region
> + * @param size Size of the memory region
> + * @param flag Memory access flag
> + */
> + rtems_status_code _Memory_protection_Set_entries(
> + uintptr_t begin_addr,
> + size_t size,
> + uint32_t flag
> +);
> +
> +/**
> + * @brief Unset the memory access permission for the specified memory region
> + * This operation implicitly sets the specified memory region with 'NO_ACCESS'
> + * flag.
> + *
> + * @param begin_addr Beginning of the memory region
> + * @param size Size of the memory region
> + */
> +rtems_status_code _Memory_protection_Unset_entries(
> + uintptr_t begin_addr,
> + size_t size
> +);
> +
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif
> \ No newline at end of file
> diff --git a/cpukit/include/rtems/score/stack.h b/cpukit/include/rtems/score/stack.h
> index df1df74867..b067d5ed1b 100644
> --- a/cpukit/include/rtems/score/stack.h
> +++ b/cpukit/include/rtems/score/stack.h
> @@ -23,6 +23,7 @@
> #define _RTEMS_SCORE_STACK_H
>
> #include <rtems/score/basedefs.h>
> +#include <rtems/confdefs.h>
>
> #ifdef __cplusplus
> extern "C" {
> @@ -47,6 +48,44 @@ extern "C" {
> */
> #define STACK_MINIMUM_SIZE CPU_STACK_MINIMUM_SIZE
>
> +#if defined (RTEMS_THREAD_STACK_PROTECTION)
> +/**
> + * The number of stacks that can be shared with a thread. Maybe this should be
> + * a configuration option provided in the rtems/confdefs.h?
> + */
> +#if CONFIGURE_MAXIMUM_TASKS > 0
> +#define SHARED_STACK_NUMBER CONFIGURE_MAXIMUM_TASKS - 1
> +#else
> +#define SHARED_STACK_NUMBER CONFIGURE_MAXIMUM_POSIX_THREADS - 1
> +#endif
> +#endif /* RTEMS_THREAD_STACK_PROTECTION */
> +
This is a little bit fragile. I think you might prefer:
#define SHARED_STACK_NUMBER ( _CONFIGURE_TASKS +
CONFIGURE_MAXIMUM_POSIX_THREADS - 1 )
> +
> +#if defined ( RTEMS_THREAD_STACK_PROTECTION )
> +
> +/**
> + * The following defines the control block of a shared stack. Each stack can have
> + * different sharing attributes.
> + */
> +typedef struct
> +{
> + /** Access flags of the shared stack */
> + uint32_t access_flags;
> + /** This is the stack address of the sharing thread*/
> + void* shared_stack_area;
> + /** Stack size of the sharing thread*/
> + size_t shared_stack_size;
> + /** This is the stack address of the target stack. Maybe this area is not
> + * needed but this helps in selecting the target thread during stack sharing.
> + */
> + void* target_stack_area;
I'm confused about the difference between shared_stack_area and
target_stack_area
> + /** Error checking for valid target stack address. This is also used to
> + * distinguish between a normal mmap operation and a stack sharing operation.
> + */
I don't really know what this means.
> + bool stack_shared;
> +} Stack_Shared_attr;
> +#endif
> +
> /**
> * The following defines the control block used to manage each stack.
> */
> @@ -55,6 +94,23 @@ typedef struct {
> size_t size;
> /** This is the low memory address of stack. */
> void *area;
> +#if defined (RTEMS_THREAD_STACK_PROTECTION)
> + /** Page table base of the thread stack*/
> + uintptr_t page_table_base;
> + /** Memory access flags of the stack. */
> + uint32_t access_flags;
> + /** The shared thread stacks with the target thread. We store the shared
> + * stacks in array, this helps in tracking them during context restoration/
> + * switch. We are using a separate structure for shared stacks, as this
> + * only has the attributes relevant to stack sharing. Having an option like
> + * Stack_Control shared_stacks[] will use up un-necessary memory.
> + */
> + Stack_Shared_attr shared_stacks[SHARED_STACK_NUMBER];
This is potentially quite large/wasteful, but OK for now. Eventually I
think this should be something that comes through configuration.
What if you used an array of pointers?
Stack_Control *shared_stacks[SHARED_STACK_NUMBER];
or kept a pointer to the Stack_Control inside of the Stack_Shared_attr
instead of the stack address/size?
> + /** The total number of thread-stacks shared with the target thread.
> + * Note -We cannot have shares_stacks_count > SHARED_STACK_NUMBER.
> + */
> + uint32_t shared_stacks_count;
> +#endif
> } Stack_Control;
>
> /**
> diff --git a/cpukit/include/rtems/score/stackprotection.h b/cpukit/include/rtems/score/stackprotection.h
> new file mode 100644
> index 0000000000..238ea64c49
> --- /dev/null
> +++ b/cpukit/include/rtems/score/stackprotection.h
> @@ -0,0 +1,80 @@
> +/**
> + * @file
> + *
> + * @ingroup RTEMSScoreStackprotection
> + *
> + * @brief Stackprotection API
fix. "Stack protection"
> + *
> + * This include file provides the API for the management of protected thread-
> + * stacks
> + */
> +
> +/*
> + * COPYRIGHT (c) 2020 Utkarsh Rai.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> + * POSSIBILITY OF SUCH DAMAGE.
> + *
> + */
> +
> +#ifndef _RTEMS_SCORE_STACKPROTECTION_H
> +#define _RTEMS_SCORE_STACKPROTECTION_H
> +
> +#include <rtems/score/basedefs.h>
> +#include <rtems/score/stack.h>
> +#include <rtems/score/memoryprotection.h>
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +/**
> + * @brief Share the stack address of a given thread with the target thread.
> + *
> + * @param target_stack Stack address of the target thread
> + * @param sharing_stack Stack address of the sharin thread
typo: sharing
2 more undocumented params
> + */
> +
> +int _Stack_protection_Share_stack(
> + void *target_stack,
> + void *sharing_stack,
I'm wondering if these should be the Stack_Control types?
> + size_t size,
> + uint32_t memory_flag
> +);
> +
> +/**
> + * @brief Swap the restored shared stacks in the page table during context
> + * restoration
> + *
> + * We set the entries of the restored stack and mark all the remaining stacks as
> + * 'NO-ACCESS'.
> + *
> + * @param Control block of the restored stack
> + */
> +void _Stack_protection_Context_restore(
> + Stack_Control *heir_stack
> +);
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif
> \ No newline at end of file
> diff --git a/cpukit/posix/src/shmopen.c b/cpukit/posix/src/shmopen.c
> index d7f494fd22..1e7fda66f8 100644
> --- a/cpukit/posix/src/shmopen.c
> +++ b/cpukit/posix/src/shmopen.c
> @@ -19,11 +19,13 @@
> #include <unistd.h>
> #include <fcntl.h>
> #include <sys/stat.h>
> +#include <string.h>
>
> #include <rtems/libio_.h>
> #include <rtems/seterr.h>
>
> #include <rtems/posix/shmimpl.h>
> +#include <rtems/posix/pthread.h>
> #include <rtems/score/wkspace.h>
> #include <rtems/sysinit.h>
>
> @@ -91,6 +93,61 @@ static int shm_ftruncate( rtems_libio_t *iop, off_t length )
> return 0;
> }
>
> +static int shm_stack_ftruncate ( rtems_libio_t *iop, off_t length )
> +{
> + int err;
> + Objects_Id id;
> + Objects_Name_or_id_lookup_errors obj_err;
> + Thread_Control *Control;
> + ISR_lock_Context lock_context;
> + size_t size;
> + char *name;
> + POSIX_Shm_Control *shm = iop_to_shm ( iop );
> +
> + name = shm->Object.name.name_p;
> +
> + /** We assign fixed pattern of naming for thread-stacks, and treat them
> + * accordingly.
> + */
> + if( strncmp( name, "/taskfs/", 8) == 0 ) {
ws
> + /**
> + * Obtain the object id of the thread and then get the thread control block
> + * corresponding to that id.
> + */
> + obj_err = _Objects_Name_to_id_u32(
> + &_POSIX_Threads_Information.Objects,
> + _Objects_Build_name( name[8], name[9], name[10], name[11]),
> + RTEMS_LOCAL,
> + &id
> + );
> + Control = _Thread_Get( id, &lock_context );
> + if( Control != NULL ) {
ws
> + shm->shm_object.handle = Control->Start.Initial_stack.area;
> + if( length != Control->Start.Initial_stack.size) {
ws
> + return ENOMEM;
> + }
> + } else {
> + return ENOMEM;
> + }
> + }else{
ws
> +
> + _Objects_Allocator_lock();
> +
> + err = (*shm->shm_object.ops->object_resize)( &shm->shm_object, length );
> +
> + if ( err != 0 ) {
> + _Objects_Allocator_unlock();
> + rtems_set_errno_and_return_minus_one( err );
> + }
> +
> + _POSIX_Shm_Update_mtime_ctime( shm );
> +
> + _Objects_Allocator_unlock();
> + return 0;
> + }
> +
> +}
> +
> static int shm_close( rtems_libio_t *iop )
> {
> POSIX_Shm_Control *shm = iop_to_shm( iop );
> @@ -306,7 +363,11 @@ static const rtems_filesystem_file_handlers_r shm_handlers = {
> .ioctl_h = rtems_filesystem_default_ioctl,
> .lseek_h = rtems_filesystem_default_lseek,
> .fstat_h = shm_fstat,
> + #if defined ( RTEMS_THREAD_STACK_PROTECTION )
> + .ftruncate_h = shm_stack_ftruncate,
> + #else
> .ftruncate_h = shm_ftruncate,
> + #endif
> .fsync_h = rtems_filesystem_default_fsync_or_fdatasync,
> .fdatasync_h = rtems_filesystem_default_fsync_or_fdatasync,
> .fcntl_h = rtems_filesystem_default_fcntl,
> diff --git a/cpukit/score/cpu/arm/cpu_asm.S b/cpukit/score/cpu/arm/cpu_asm.S
> index 66f8ba6032..36b6c50f91 100644
> --- a/cpukit/score/cpu/arm/cpu_asm.S
> +++ b/cpukit/score/cpu/arm/cpu_asm.S
> @@ -66,6 +66,73 @@ DEFINE_FUNCTION_ARM(_CPU_Context_switch)
>
> str r3, [r0, #ARM_CONTEXT_CONTROL_ISR_DISPATCH_DISABLE]
>
> +#if defined ( RTEMS_THREAD_STACK_PROTECTION )
> +
> +/*
> + * We make a function call to set the memory attributes of the heir
> + * stack and unset that of the executing stack (including shared stacks ).
> + * This will be a simple asm code when done by switching the translation
> + * table base.
> + */
> +
> +/* Save the registers modified during function call */
> + mov r8, r0
> + mov r9, r1
> + mov r10, r2
> +/* Load the parameters for function call */
> + mov r0, r1
> + sub r0, r0, #102
> + ldr r1, [r0]
> + ldr r0, [r0, #4]
> + mov r2, #3
> + bl _Memory_protection_Set_entries
> +
> +/* Restore the saved registers */
> + mov r0, r8
> + mov r1, r9
> + mov r2, r10
> +
> +/* We load the stack-pointer of the heir thread pre-maturely, this is
> + * done as oherwise the *_unset_entries call will execute from the stack of the
> + * executing thread causing fatal exceptions.
> + */
> + ldr sp, [r1, #32]
> +/* Load the parameters of the function call */
> + mov r1, r0
> + sub r0, r0, #102
> + ldr r1, [r0]
> + ldr r0, [r0, #4]
> + bl _Memory_protection_Unset_entries
> +/*
> + * Restore the saved registers
> + */
> + mov r0, r8
> + mov r1, r9
> + mov r2, r10
> +
> +/* Unset the memory entries of the shared stack */
> + sub r0, r0, #102
> + add r6, r0, #16
> +
> +.L_shared_stack_restore:
> +
> + ldr r1, [r6, #4]
> + ldr r0, [r6, #8]
> + bl _Memory_protection_Unset_entries
> + add r6, #17
> + add r11, #1
> +/* We compare with a hard-coded value, this is the number of shared stacks
> + * (SHARED_STACK_NUMBER)
> + */
> + cmp r11, #8
It should be possible to get the SHARED_STACK_NUMBER macro through to asm.
> + bne .L_shared_stack_restore
> +
> +/* Restore the saved registers */
> + mov r0, r8
> + mov r1, r9
> + mov r2, r10
> +#endif
> +
> #ifdef RTEMS_SMP
> /*
> * The executing thread no longer executes on this processor. Switch
> @@ -95,6 +162,7 @@ DEFINE_FUNCTION_ARM(_CPU_Context_switch)
>
> /* Start restoring context */
> .L_restore:
> +
avoid random changes in existing code.
> #if !defined(RTEMS_SMP) && defined(ARM_MULTILIB_HAS_LOAD_STORE_EXCLUSIVE)
> clrex
> #endif
> @@ -133,7 +201,33 @@ DEFINE_FUNCTION_ARM(_CPU_Context_switch)
> */
> DEFINE_FUNCTION_ARM(_CPU_Context_restore)
> mov r1, r0
> +
ditto
> GET_SELF_CPU_CONTROL r2
> +
> +#if defined ( RTEMS_THREAD_STACK_PROTECTION )
> +
> + /* Save the registers modified during function call */
> + mov r8, r0
> + mov r9, r1
> + mov r10, r2
> +
> + /* Load the parameters for function call */
> + mov r1, r0
> + sub r0, r0, #102
> + ldr r1, [r0]
> + ldr r0, [r0, #4]
> + mov r2, #3
> + bl _Memory_protection_Set_entries
> +
> + /* Restore the saved registers */
> + mov r0, r8
> + mov r1, r9
> + mov r2, r10
> +
> + /* Set memory entries of the shared thread stacks */
> +
> +
> +#endif
> b .L_restore
>
> #ifdef RTEMS_SMP
> diff --git a/cpukit/score/cpu/arm/include/rtems/score/cpu.h b/cpukit/score/cpu/arm/include/rtems/score/cpu.h
> index b90fb1f394..28d5870856 100644
> --- a/cpukit/score/cpu/arm/include/rtems/score/cpu.h
> +++ b/cpukit/score/cpu/arm/include/rtems/score/cpu.h
> @@ -138,7 +138,7 @@
> /* AAPCS, section 5.2.1.2, Stack constraints at a public interface */
> #define CPU_STACK_ALIGNMENT 8
>
> -#define CPU_INTERRUPT_STACK_ALIGNMENT CPU_CACHE_LINE_BYTES
> +#define CPU_INTERRUPT_STACK_ALIGNMENT 4096
>
> /*
> * Bitfield handler macros.
> diff --git a/cpukit/score/src/stackprotection.c b/cpukit/score/src/stackprotection.c
> new file mode 100644
> index 0000000000..cf2e44e63d
> --- /dev/null
> +++ b/cpukit/score/src/stackprotection.c
> @@ -0,0 +1,125 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +
> +/**
> + * @file
> + *
> + * @ingroup RTEMSScoreStackprotection
> + *
> + */
> +
> +/*
> + * Copyright (C) 2020 Utkarsh Rai
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> + * POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +
only 1 blank line
> +#ifdef HAVE_CONFIG_H
> +#include "config.h"
> +#endif
> +
> +#include <rtems.h>
> +#include <rtems/score/stackprotection.h>
> +#include <rtems/score/threadimpl.h>
> +
> +void _Stack_protection_Context_restore(
> +Stack_Control *heir_stack
> +)
> +{
> + void* stack_address;
> + size_t stack_size;
> + uint32_t memory_flags;
> + uint32_t index;
> +
> + for(index = 0;index < heir_stack->shared_stacks_count; index++) {
ws
> + stack_address = heir_stack->shared_stacks[index].shared_stack_area;
> + stack_size = heir_stack->shared_stacks[index].shared_stack_size;
> + _Memory_protection_Set_entries( stack_address, stack_size, memory_flags );
> + }
> +
> +}
> +
> +static bool get_target_thread( Thread_Control *Control, void *arg)
> +{
> + Stack_Shared_attr *shared_stack;
> + uint32_t count;
> + shared_stack = arg;
why not pass (arg) as Stack_Shared_attr*?
> + rtems_status_code err;
> + /**
> + * Check for the target thread by comparing the stack address. Add the shared stack
> + * attribute structure to the array tracking all the shared stacks.
> + */
> + if( Control->Start.Initial_stack.area == shared_stack->target_stack_area) {
> + count = Control->Start.Initial_stack.shared_stacks_count + 1;
why add 1?
> + if(count >= SHARED_STACK_NUMBER) {
couldn't this be equal?
> + return false;
> + }
> + Control->Start.Initial_stack.shared_stacks[count].access_flags =
> + shared_stack->access_flags;
indentations should add 2 nesting levels
> + Control->Start.Initial_stack.shared_stacks[count].shared_stack_area =
> + shared_stack->shared_stack_area;
> + Control->Start.Initial_stack.shared_stacks[count].shared_stack_size =
> + shared_stack->shared_stack_size;
> + Control->Start.Initial_stack.shared_stacks[count].target_stack_area =
> + shared_stack->target_stack_area;
This would be much simpler by using pointers. The above could also be done as
*(Control->Start.Initial_stack.shared_stacks[count]) = *shared_stack;
> + Control->Start.Initial_stack.shared_stacks_count = count;
> +
> + err = _Memory_protection_Set_entries(
> + shared_stack->shared_stack_area,
> + shared_stack->shared_stack_size,
> + shared_stack->access_flags );
close parens on own line
> +
> + if( err = RTEMS_SUCCESSFUL ) {
This is always true (need ==)
> + Control->Start.Initial_stack.shared_stacks[count].stack_shared =
> + shared_stack->stack_shared = true;
indent
> +
> + return true;
> + } else {
> + return false;
> + }
> +
use return false here and remove the else clause
Any reason to ignore the actual err code?
> + }
> +}
> +
> +int _Stack_protection_Share_stack(
> + void* target_stack,
> + void* sharing_stack,
> + size_t size,
The size is an attribute of the target stack. It shouldn't need to be
an argument?
> + uint32_t memory_flag
> +)
> +{
> + Thread_Control *target_thread;
indent 2 spaces
> + Stack_Shared_attr shared_stack;
> +
> + shared_stack.access_flags= memory_flag;
> + shared_stack.shared_stack_area = sharing_stack;
> + shared_stack.target_stack_area = target_stack;
> + shared_stack.shared_stack_size = size;
> + shared_stack.stack_shared = false;
> +
> + _Thread_Iterate( get_target_thread, &shared_stack );
Why doesn't the caller know the thread?
> + if( shared_stack.stack_shared == true ) {
> + return 0;
> + } else {
> + return -1;
maybe this should be an error code / status?
> + }
> +}
> \ No newline at end of file
> diff --git a/cpukit/score/src/threadhandler.c b/cpukit/score/src/threadhandler.c
> index 6742b0b391..de5ba8ea3b 100644
> --- a/cpukit/score/src/threadhandler.c
> +++ b/cpukit/score/src/threadhandler.c
> @@ -22,6 +22,7 @@
> #include <rtems/score/assert.h>
> #include <rtems/score/interr.h>
> #include <rtems/score/isrlevel.h>
> +#include <rtems/score/stackprotection.h>
> #include <rtems/score/userextimpl.h>
>
> /*
> @@ -94,6 +95,11 @@ void _Thread_Handler( void )
> level = executing->Start.isr_level;
> _ISR_Set_level( level );
>
> + /*
> + * Switch-out the shared thread-stacks
> + */
> + _Stackprotection_Context_restore( &executing->Start.Initial_stack );
> +
> /*
> * Initialize the floating point context because we do not come
> * through _Thread_Dispatch on our first invocation. So the normal
> diff --git a/testsuites/samples/thread_stack_protection/init.c b/testsuites/samples/thread_stack_protection/init.c
> new file mode 100644
> index 0000000000..d24d13850a
> --- /dev/null
> +++ b/testsuites/samples/thread_stack_protection/init.c
The test name doesn't match the general pattern.
> @@ -0,0 +1,114 @@
> +#ifdef HAVE_CONFIG_H
> +#include "config.h"
> +#endif
> +
> +#include <rtems.h>
> +#include <tmacros.h>
> +#include <pthread.h>
> +#include <rtems/score/memoryprotection.h>
> +#include <rtems/score/thread.h>
> +const char rtems_test_name[] = " THREAD STACK PROTECTION ";
> +
> +static void fatal_extension(
> + rtems_fatal_source source,
> + bool is_internal,
> + rtems_fatal_code error
> +)
> +{
> + if (source == RTEMS_FATAL_SOURCE_EXCEPTION)
> + {
align with if
> + printk("Internal Exception Occured \n");
will this work?
too far indented
> + }
> +
> + exit(0);
> +}
> +
What tests did you look at to implement this test?
If it intends to fail, you should check out the spfatal* tests.
> +void* Test_routine( void* arg )
> +{
> +
> +}
> +
> +void *POSIX_Init( void *argument )
If the test requires/uses POSIX, then it should be a psxtest.
You probably do want to try to write a couple of sptests that show a
few different cases, at a minimum
* two tasks sharing each others stacks successfully
* two tasks not sharing with each other and causing an exception
> +{
> + void *stack_addr1;
> + void *stack_addr2;
> + size_t stack_size1;
> + size_t stack_size2;
> + pthread_t id1;
> + pthread_t id2;
> + pthread_attr_t attr1;
> + pthread_attr_t attr2;
> + Thread_Control Control;
> + TEST_BEGIN();
> +
> + /*
> + * We set the stack size as 8Kb.
> + */
> + stack_size1 = 8192;
> + stack_size2 = 8192;
> + printf("%d %d \n",sizeof(Control.Post_switch_actions), sizeof(Control.Start.tls_area));
> + /*
> + * We allocate page-aligned memory of the stack from the application.
> + */
> + posix_memalign(&stack_addr1, sysconf( _SC_PAGESIZE ), stack_size1 );
> + posix_memalign(&stack_addr2, sysconf( _SC_PAGESIZE ), stack_size2 );
> +
> + pthread_attr_init( &attr1 );
> + pthread_attr_init( &attr2 );
> +
> + /*
> + * We set the stack size and address of the thread from the application itself
> + */
> + pthread_attr_setstack( &attr1, stack_addr1, stack_size1 );
> + pthread_attr_setstack( &attr2, stack_addr2, stack_size2 );
> +
> + pthread_create( &id1, &attr1, Test_routine, NULL );
> + pthread_create( &id2, &attr2, Test_routine, NULL );
> + /*
> + * We set the memory attributes of the stack from the application.
> + */
> + _Memory_protection_Set_entries( stack_addr1, stack_size1, RTEMS_NO_ACCESS | RTEMS_MEMORY_CACHED );
> + _Memory_protection_Set_entries( stack_addr2, stack_size2, RTEMS_NO_ACCESS | RTEMS_MEMORY_CACHED );
> +
> + pthread_join( id1, NULL );
> + /*
> + * Write to the stack address of thread1 after it has been switched out.
> + */
> + printf("Writing to the stack of thread1 \n");
> + //memset(stack_addr1, 0, stack_size1);
> +
> + pthread_join( id2, NULL );
> + /*
> + * Write to the stack address of thread2 after it has been switched out.
> + */
> +
> +
> +
> + TEST_END();
> + rtems_test_exit( 0 );
> +}
> +
> +/* configuration information */
> +
> +#define CONFIGURE_INIT
> +
> +#define CONFIGURE_APPLICATION_NEEDS_SIMPLE_CONSOLE_DRIVER
> +#define CONFIGURE_APPLICATION_NEEDS_CLOCK_DRIVER
> +
> +#define CONFIGURE_INITIAL_EXTENSIONS RTEMS_TEST_INITIAL_EXTENSION
> +
> +#define CONFIGURE_MAXIMUM_POSIX_THREADS 2
> +
> +#define CONFIGURE_POSIX_INIT_THREAD_TABLE
> +
> +#define CPU_INTERRUPT_STACK_ALIGNMENT 4096
> +
> +#define CONFIGURE_INITIAL_EXTENSIONS { .fatal = fatal_extension }
> +
> +#define CONFIGURE_TASK_STACK_ALLOCATOR_INIT bsp_stack_allocate_init
> +#define CONFIGURE_TASK_STACK_ALLOCATOR bsp_stack_allocate
> +#define CONFIGURE_TASK_STACK_DEALLOCATOR bsp_stack_free
> +
> +#include <bsp/stackalloc.h>
> +#define CONFIGURE_INIT
> +#include <rtems/confdefs.h>
> \ No newline at end of file
> diff --git a/testsuites/samples/thread_stack_protection/thread_stack_protection.doc b/testsuites/samples/thread_stack_protection/thread_stack_protection.doc
> new file mode 100644
> index 0000000000..c5c9cdfa81
> --- /dev/null
> +++ b/testsuites/samples/thread_stack_protection/thread_stack_protection.doc
> @@ -0,0 +1,2 @@
> +This test sample demonstrates the thread stack protection functionality. We try
> +to write to the stack of a switched-out thread.
> \ No newline at end of file
> diff --git a/testsuites/samples/thread_stack_protection/thread_stack_protection.scn b/testsuites/samples/thread_stack_protection/thread_stack_protection.scn
> new file mode 100644
> index 0000000000..1d36d0dfaf
> --- /dev/null
> +++ b/testsuites/samples/thread_stack_protection/thread_stack_protection.scn
> @@ -0,0 +1,20 @@
> +
> +*** BEGIN OF TEST THREAD STACK PROTECTION ***
> +*** TEST VERSION: 5.0.0.a3b39c9e567f3f1ef5ab01de78b113e61b11853b-modified
> +*** TEST STATE: EXPECTED_PASS
> +*** TEST BUILD: RTEMS_NETWORKING RTEMS_POSIX_API
> +*** TEST TOOLS: 7.5.0 20191114 (RTEMS 5, RSB 5 (3bd11fd4898b), Newlib 7947581)
> +Creating thread1
> +Creating thread2
> +Joining thread1
> +Writing to the stack of thread1
> +Internal Exception Occured
> +Internal Exception Occured
> +Internal Exception Occured
> +Internal Exception Occured
> +Internal Exception Occured
> +Internal Exception Occured
> +Internal Exception Occured
> +Internal Exception Occured
> +Internal Exception Occured
> +Internal Exception Occured
> \ No newline at end of file
> --
> 2.17.1
>
More information about the devel
mailing list