[PATCH] Strict thread-stack isolation and thread-stack sharing.
Gedare Bloom
gedare at rtems.org
Fri Jul 31 19:13:02 UTC 2020
On Fri, Jul 31, 2020 at 11:13 AM Utkarsh Rai <utkarsh.rai60 at gmail.com> wrote:
>
> This patch provides thread-stack isolation and thread-stack sharing
> mechanism for a POSIX thread.
>
> - Thread stacks are isolated with read-only permission.
> - We use mmap, shm_open for thread-stack sharing. For mechanism of
> thread-stack sharing please refer to the related test.
> ---
> .../realview-pbx-a9/mmu/bsp-set-mmu-attr.c | 77 +++++++++
> bsps/shared/start/stackalloc.c | 19 ++-
> .../libbsp/arm/realview-pbx-a9/Makefile.am | 3 +
> c/src/lib/libbsp/arm/xilinx-zynq/Makefile.am | 2 +-
> cpukit/Makefile.am | 2 +
> cpukit/headers.am | 3 +
> cpukit/include/rtems/score/memoryprotection.h | 82 ++++++++++
> cpukit/include/rtems/score/stackprotection.h | 151 ++++++++++++++++++
> cpukit/include/rtems/score/thread.h | 8 +
> cpukit/include/rtems/stackname.h | 71 ++++++++
> cpukit/posix/src/mmap.c | 112 ++++++++++++-
> cpukit/posix/src/shmwkspace.c | 24 +++
> cpukit/rtems/src/stackname.c | 102 ++++++++++++
> cpukit/score/cpu/arm/cpu.c | 2 +-
> cpukit/score/cpu/arm/cpu_asm.S | 64 +++++++-
> .../score/cpu/arm/include/rtems/score/cpu.h | 2 +-
> cpukit/score/src/stackprotection.c | 108 +++++++++++++
> cpukit/score/src/threadhandler.c | 6 +
> cpukit/score/src/threadinitialize.c | 23 ++-
> testsuites/psxtests/psx16/init.c | 2 +-
> testsuites/samples/hello/init.c | 2 +-
> .../samples/thread_stack_protection/init.c | 89 +++++++++++
> .../thread_stack_protection.doc | 2 +
> .../thread_stack_protection.scn | 19 +++
> .../samples/thread_stack_sharing/init.c | 121 ++++++++++++++
> 25 files changed, 1078 insertions(+), 18 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/include/rtems/stackname.h
> create mode 100644 cpukit/rtems/src/stackname.c
> 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
> create mode 100644 testsuites/samples/thread_stack_sharing/init.c
>
> 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..4924fbb973
> --- /dev/null
> +++ b/bsps/arm/realview-pbx-a9/mmu/bsp-set-mmu-attr.c
> @@ -0,0 +1,77 @@
> +#include <bsp/arm-cp15-start.h>
> +#include <rtems/score/memoryprotection.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;
> +
> + /*
> + * Access permission
> + */
> + memory_attribute = ( attr_flags & READ_WRITE ) | ( attr_flags & READ_ONLY ) |
> + ( attr_flags & NO_ACCESS );
> +
> + switch (memory_attribute)
> + {
> + case READ_WRITE:
> + flags = ARMV7_MMU_READ_WRITE;
> + break;
> +
> + case READ_ONLY:
> + flags = ARMV7_MMU_READ_ONLY;
> + break;
> +
> + case NO_ACCESS:
> + default:
> + flags = 0;
> + break;
> + }
> +
> + /*
> + * Check for memory-cache operation
> + */
> + if( attr_flags & MEMORY_CACHED ) {
> + flags |= ARM_MMU_SECT_TEX_0 | ARM_MMU_SECT_C | ARM_MMU_SECT_B;
> + }
> +
> + return flags;
> +}
> +
> +void _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;
> + access_flags = translate_flags(flags);
> +
> + /*
> + * 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);
> +}
> +
> +void _Memory_protection_Unset_entries(uintptr_t begin, size_t size)
> +{
> + uint32_t access_flags;
> + uintptr_t end;
> + rtems_interrupt_level irq_level;
> +
> + end = begin + size;
> + access_flags = translate_flags(READ_ONLY);
why isn't it NO_ACCESS?
> +
> + /*
> + * 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);
> +}
Should this _Memory_protection_Unset_entries() ever be different than
_Memory_protection_Set_entries(begin, size, NO_ACCESS)?
Anyway, it could be implemented here as a call to Set_entries()
> \ No newline at end of file
> diff --git a/bsps/shared/start/stackalloc.c b/bsps/shared/start/stackalloc.c
> index f7cf7be0f1..777e57aac6 100644
> --- a/bsps/shared/start/stackalloc.c
> +++ b/bsps/shared/start/stackalloc.c
> @@ -23,11 +23,15 @@
> #include <bsp/stackalloc.h>
>
> #include <rtems.h>
> +#include <rtems/score/basedefs.h>
> #include <rtems/score/heapimpl.h>
> #include <rtems/score/wkspace.h>
> +#include <rtems/score/memoryprotection.h>
>
> #include <bsp/linker-symbols.h>
>
> +#define USE_THREAD_STACK_PROTECTION
You need to figure out how to get this to come through the configuration system.
> +
> static Heap_Control bsp_stack_heap;
>
> void bsp_stack_allocate_init(size_t stack_space_size)
> @@ -44,6 +48,15 @@ void *bsp_stack_allocate(size_t size)
> {
> void *stack = NULL;
>
> +#if defined (USE_THREAD_STACK_PROTECTION)
> +/*
> + * This is a temporary hack, we need to use _Heap_Allocate_aligned() but the heap
> + * intialization for bsp_stack_heap fails as bsp_section_stack_size is 0. See
> + * bsp_stack_allocate_init().
> + */
> + posix_memalign(&stack, 4096 , size);
I guess the size != 4096 can cause other dynamic memory to be
allocated in the same page as this stack, and could make problems. For
now, I'd set the size to 4096.
This 4096 should come through a PGSIZE kind of macro. It could maybe
be CPU_PAGE_SIZE or something, we might need to add it to the CPU
Supplement for all architectures.
> + _Memory_protection_Set_entries( stack, size, ( READ_ONLY | MEMORY_CACHED ) );
> +#else
> if (bsp_stack_heap.area_begin != 0) {
> stack = _Heap_Allocate(&bsp_stack_heap, size);
> }
> @@ -51,15 +64,15 @@ void *bsp_stack_allocate(size_t size)
> if (stack == NULL) {
> stack = _Workspace_Allocate(size);
> }
> -
> +#endif
Add a comment on the endif to refer the condition, e.g.,
#endif /* USE_THREAD_STACK_PROTECTION */
> return stack;
> }
>
> -void bsp_stack_free(void *stack)
> +void bsp_stack_free(void *stack)
spurious change?
> {
> bool ok = _Heap_Free(&bsp_stack_heap, stack);
>
> if (!ok) {
> _Workspace_Free(stack);
> }
> -}
> +}
?
> \ No newline at end of file
> diff --git a/c/src/lib/libbsp/arm/realview-pbx-a9/Makefile.am b/c/src/lib/libbsp/arm/realview-pbx-a9/Makefile.am
> index d5549275be..24847c7b91 100644
> --- a/c/src/lib/libbsp/arm/realview-pbx-a9/Makefile.am
> +++ b/c/src/lib/libbsp/arm/realview-pbx-a9/Makefile.am
> @@ -74,6 +74,9 @@ librtemsbsp_a_SOURCES += ../../../../../../bsps/arm/shared/clock/clock-a9mpcore.
> librtemsbsp_a_SOURCES += ../../../../../../bsps/arm/shared/cache/cache-cp15.c
> librtemsbsp_a_SOURCES += ../../../../../../bsps/arm/shared/cache/cache-v7ar-disable-data.S
>
> +#MMU
> +librtemsbsp_a_SOURCES += ../../../../../../bsps/arm/realview-pbx-a9/mmu/bsp-set-mmu-attr.c
> +
> # Start hooks
> librtemsbsp_a_SOURCES += ../../../../../../bsps/arm/realview-pbx-a9/start/bspstarthooks.c
>
> diff --git a/c/src/lib/libbsp/arm/xilinx-zynq/Makefile.am b/c/src/lib/libbsp/arm/xilinx-zynq/Makefile.am
> index cfd59475c2..34adebc503 100644
> --- a/c/src/lib/libbsp/arm/xilinx-zynq/Makefile.am
> +++ b/c/src/lib/libbsp/arm/xilinx-zynq/Makefile.am
> @@ -85,4 +85,4 @@ librtemsbsp_a_SOURCES += ../../../../../../bsps/arm/xilinx-zynq/start/bspstartmm
>
> include $(srcdir)/../../../../../../bsps/shared/irq-sources.am
> include $(srcdir)/../../../../../../bsps/shared/shared-sources.am
> -include $(srcdir)/../../../../../../bsps/arm/xilinx-zynq/headers.am
> +include $(srcdir)/../../../../../../bsps/arm/xilinx-zynq/headers.am
> \ No newline at end of file
> diff --git a/cpukit/Makefile.am b/cpukit/Makefile.am
> index 51f38c84c7..3d50b884e2 100644
> --- a/cpukit/Makefile.am
> +++ b/cpukit/Makefile.am
> @@ -779,6 +779,7 @@ librtemscpu_a_SOURCES += rtems/src/semrelease.c
> librtemscpu_a_SOURCES += rtems/src/semsetpriority.c
> librtemscpu_a_SOURCES += rtems/src/signalcatch.c
> librtemscpu_a_SOURCES += rtems/src/signalsend.c
> +librtemscpu_a_SOURCES += rtems/src/stackname.c
> librtemscpu_a_SOURCES += rtems/src/status.c
> librtemscpu_a_SOURCES += rtems/src/statustext.c
> librtemscpu_a_SOURCES += rtems/src/statustoerrno.c
> @@ -929,6 +930,7 @@ librtemscpu_a_SOURCES += score/src/schedulercbsgetserverid.c
> librtemscpu_a_SOURCES += score/src/schedulercbssetparameters.c
> librtemscpu_a_SOURCES += score/src/schedulercbsreleasejob.c
> librtemscpu_a_SOURCES += score/src/schedulercbsunblock.c
> +librtemscpu_a_SOURCES += score/src/stackprotection.c
> librtemscpu_a_SOURCES += score/src/stackallocator.c
> librtemscpu_a_SOURCES += score/src/pheapallocate.c
> librtemscpu_a_SOURCES += score/src/pheapextend.c
> diff --git a/cpukit/headers.am b/cpukit/headers.am
> index fcf679f09d..57c90a665f 100644
> --- a/cpukit/headers.am
> +++ b/cpukit/headers.am
> @@ -162,6 +162,7 @@ include_rtems_HEADERS += include/rtems/shellconfig.h
> include_rtems_HEADERS += include/rtems/sparse-disk.h
> include_rtems_HEADERS += include/rtems/spurious.h
> include_rtems_HEADERS += include/rtems/stackchk.h
> +include_rtems_HEADERS += include/rtems/stackname.h
> include_rtems_HEADERS += include/rtems/status-checks.h
> include_rtems_HEADERS += include/rtems/stdio-redirect.h
> include_rtems_HEADERS += include/rtems/stringto.h
> @@ -352,6 +353,7 @@ include_rtems_score_HEADERS += include/rtems/score/isr.h
> include_rtems_score_HEADERS += include/rtems/score/isrlevel.h
> include_rtems_score_HEADERS += include/rtems/score/isrlock.h
> include_rtems_score_HEADERS += include/rtems/score/memory.h
> +include_rtems_score_HEADERS += include/rtems/score/memoryprotection.h
> include_rtems_score_HEADERS += include/rtems/score/mpci.h
> include_rtems_score_HEADERS += include/rtems/score/mpciimpl.h
> include_rtems_score_HEADERS += include/rtems/score/mppkt.h
> @@ -405,6 +407,7 @@ include_rtems_score_HEADERS += include/rtems/score/smplockstats.h
> include_rtems_score_HEADERS += include/rtems/score/smplockticket.h
> include_rtems_score_HEADERS += include/rtems/score/stack.h
> include_rtems_score_HEADERS += include/rtems/score/stackimpl.h
> +include_rtems_score_HEADERS += include/rtems/score/stackprotection.h
> include_rtems_score_HEADERS += include/rtems/score/states.h
> include_rtems_score_HEADERS += include/rtems/score/statesimpl.h
> include_rtems_score_HEADERS += include/rtems/score/status.h
> diff --git a/cpukit/include/rtems/score/memoryprotection.h b/cpukit/include/rtems/score/memoryprotection.h
> new file mode 100644
> index 0000000000..cc01f8ba9b
> --- /dev/null
> +++ b/cpukit/include/rtems/score/memoryprotection.h
> @@ -0,0 +1,82 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +
> +/**
> + * @file
> + *
> + * @ingroup RTEMSScoreMemoryprotection
use camelCase--capital P
> + *
> + * @brief This file provodes APIs for high-level memory protection
typo
> + *
> + */
> +
> +/*
> + * 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>
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#define NO_ACCESS 0x00
> +#define READ_ONLY 0x01
> +#define READ_WRITE 0x02
> +#define MEMORY_CACHED 0x04
I'm not sure about these. Maybe, they should be an inclusive bit
field, so it should be.
#define READ 0x01
#define WRITE 0x02
then you can get
#define READ_WRITE (READ|WRITE)
we might want to put these macros in a namespace also. This is less
important than for symbols, but it can still be good to try to avoid
clashing with user symbols/macros. Maybe:
RTEMS_MEMORY_READ
or something.
> +
> +/**
> + * @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
> + *
get rid of the blank line before the end of the comment
> + */
> +void _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 Begining of the memory region
> + * @param size Size of the memory region
> + */
> +void _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/stackprotection.h b/cpukit/include/rtems/score/stackprotection.h
> new file mode 100644
> index 0000000000..60893da008
> --- /dev/null
> +++ b/cpukit/include/rtems/score/stackprotection.h
> @@ -0,0 +1,151 @@
> +/**
> + * @file
> + *
> + * @ingroup RTEMSScoreStackprotection
> + *
> + * @brief Stackprotection API
Use a phrase
> + *
> + * 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
> +
> +#if defined ( ASM )
> + #include <rtems/asm.h>
> +#else
> + #include <rtems/score/basedefs.h>
> + #include <rtems/score/chainimpl.h>
> + #include <rtems/score/memoryprotection.h>
> + #include <rtems/score/stack.h>
> +#endif
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#if !defined ( ASM )
> +
> +/**
> + * The following defines the attributes of a protected stack
> + */
> +typedef struct
> +{
> + /** The stack address */
> + uintptr_t stack_address;
> + /** This is the stack size */
> + size_t size;
> + /** The pointer to the page table base */
> + uintptr_t page_table_base;
> + /**Memory flag for the alllocated/shared stack */
typo
> + uint32_t access_flags;
> +} Stackprotection_Attr;
Stack_protection_Attr
> +
> +/**
> + * The following defines the control block of a shared stack
> + */
> +typedef struct
> +{
> + /** This is the chain node for tracking shared stacks */
> + Chain_Node node;
> + /** This is the attribute of a shared stack*/
> + Stackprotection_Attr Base;
You haven't responded to my previous complaints about the use of a
'Base' that isn't located at the start of a structure.
> +} Stackprotection_Shared_stack;
> +
> +/**
> + * The following defines the control block of an allocated stack
> + */
> +typedef struct
> +{
> + /** The attribute of an allocated stack*/
> + Stackprotection_Attr Base;
> + /** The pointer to the attributes of a stack shared with the stack
> + * in question
> + */
> + Stackprotection_Shared_stack *shared_stacks;
> + /*
> + * Name of the thread-stack
> + */
> + char *name;
> + /**
> + * The chain control for tracking the shared stacks with the thread-stack in
> + * question.
> + */
> + Chain_Control shared_stack_control;
> +} Stackprotection_Stack;
> +
> +/**
> + * @brief Share the stack of a stack with the specified thread.
'stack of a stack' is confusing to me
> + *
> + * @param shared_address The stack to be shared
> + * @param target_address The stack with which to share
> + * @param flags The memory-access flag of the shared stack address
> + */
> +void _Stackprotection_Share_stack(
> + Stackprotection_Stack *shared_stack,
> + Stackprotection_Stack *target_stack,
> + size_t shared_stack_size,
> + uint32_t flag
> +);
> +
> +/**
> + * @brief Swap out the executing shared stack from the page table during
> + * context switch
> + *
> + * The current method of switching the protected stack is to mark the switched
> + * out stack as 'NO ACCESS'
> + *
> + * @param excuting_stack Control block of the executing stack
> + */
> +void _Stackprotection_Context_switch(
> + Stackprotection_Stack *executing_stack,
> + Stackprotection_Stack *heir_stack
> +);
> +
I would like you to please engage in discussion about the design of
your approach. Too much is being lost during these code reviews. I
make comments related to design, and they don't get answered, and then
you send a new patch, and we haven't resolved the questions.
Consolidate the design and the questions I raised previously, and
let's try to figure out if the design you've come up with is useable
or how to proceed next.
I'm opposed to having function calls in the context switch path, if we
can avoid it.
> +/**
> + * @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 remainig stacks as
> + * 'NO-ACCESS'.
> + *
> + * @param Control block of the restored stack
> + */
> +void _Stackprotection_Context_restore(
> + Stackprotection_Stack *heir_stack
> +);
> +
> +#endif /* !defined ( ASM ) */
There's nothing in this file for ASM include, why would this be
included from ASM?
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif
> \ No newline at end of file
> diff --git a/cpukit/include/rtems/score/thread.h b/cpukit/include/rtems/score/thread.h
> index 2e7380f99a..728292123a 100644
> --- a/cpukit/include/rtems/score/thread.h
> +++ b/cpukit/include/rtems/score/thread.h
> @@ -34,6 +34,7 @@
> #include <rtems/score/priority.h>
> #include <rtems/score/schedulernode.h>
> #include <rtems/score/stack.h>
> +#include <rtems/score/stackprotection.h>
> #include <rtems/score/states.h>
> #include <rtems/score/threadq.h>
> #include <rtems/score/timestamp.h>
> @@ -85,6 +86,8 @@ extern "C" {
> */
> #define RTEMS_SCORE_THREAD_ENABLE_SCHEDULER_CALLOUT
>
> +#define USE_THREAD_STACK_PROTECTION
> +
multiply defined?
> #if defined(RTEMS_DEBUG)
> #define RTEMS_SCORE_THREAD_ENABLE_RESOURCE_COUNT
> #endif
> @@ -835,6 +838,11 @@ struct _Thread_Control {
>
> /** This field contains the context of this thread. */
> Context_Control Registers;
> +
> +#if defined (USE_THREAD_STACK_PROTECTION)
> + /** This is the control block of the protected stack. */
> + Stackprotection_Stack the_stack;
You still haven't explained to me why you don't extend the existing
Stack_Control structure.
> +#endif
> #if ( CPU_HARDWARE_FP == TRUE ) || ( CPU_SOFTWARE_FP == TRUE )
> /** This field points to the floating point context for this thread.
> * If NULL, the thread is integer only.
> diff --git a/cpukit/include/rtems/stackname.h b/cpukit/include/rtems/stackname.h
> new file mode 100644
> index 0000000000..bd59c98ef1
> --- /dev/null
> +++ b/cpukit/include/rtems/stackname.h
> @@ -0,0 +1,71 @@
> +/**
> + * @file
> + *
> + * @brief Stack name API
> + */
> +
> +/*
> + * 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.
> + *
delete extra blank line
> + */
> +
> +#ifndef _RTEMS_STACKNAME_H
> +#define _RTEMS_STACKNAME_H
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +/**
> + * @defgroup
> + *
> + * @ingroup
> + *
> + * @brief Stack name API
> + */
> +/**
> + * @brief Get the name of the provided stack address.
> + *
> + * @param name The name of the given stack address.
> + *
> + * @retval NULL @a stack_name is not present in the chain.
> + * @retval address @a stack_address.
> + */
> +void *rtems_stack_address_get( char* name );
> +
Why does a stack need a name different from its owning thread?
> +/**
> + * @brief Get the name of the provided stack address.
> + *
> + * @param stack_address The address of the given stack.
> + *
> + * @retval NULL @a stack_address is not present in the chain.
> + * @retval name @a stack_address name.
> + */
> +char *rtems_stack_name_get( uintptr_t stack_address );
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif
> \ No newline at end of file
> diff --git a/cpukit/posix/src/mmap.c b/cpukit/posix/src/mmap.c
> index 176c6e4fe8..99c3ea4414 100644
> --- a/cpukit/posix/src/mmap.c
> +++ b/cpukit/posix/src/mmap.c
> @@ -28,7 +28,49 @@
>
> #include <rtems/posix/mmanimpl.h>
> #include <rtems/posix/shmimpl.h>
> +#include <rtems/score/stackprotection.h>
>
> +#define USE_THREAD_STACK_PROTECTION
> +
> +#if defined(USE_THREAD_STACK_PROTECTION)
> + Stackprotection_Stack *target_stack;
> + Stackprotection_Stack *shared_stack;
No, you can't use these global variables. Global variables are
race-condition prone and they pollute the user application namespace.
We minimize the use of globals wherever possible.
What do you need them for?
> +
> +static uint32_t mmap_flag_translate(int prot)
> +{
> + int prot_read;
> + int prot_write;
> + int memory_flag;
> +
> + prot_read = (prot_read & PROT_READ) == PROT_READ;
> + prot_write = (prot_write & PROT_WRITE) == PROT_WRITE;
> +
> + if(prot_read){
Fix white space
> + memory_flag |= ( READ_ONLY| MEMORY_CACHED );
> + }
> + if(prot_write) {
> + memory_flag |= ( READ_WRITE | MEMORY_CACHED );
> + }
CACHED attribute should probably be handled separately?
> +
> + return memory_flag;
> +}
> +
> +static bool get_target_thread_visitor(Thread_Control *the_thread, void* arg)
> +{
> + if(the_thread->Start.Initial_stack.area == arg) {
> + target_stack = &the_thread->the_stack;
> + return true;
> + }
no return value?
> +}
> +
> +static bool get_shared_thread_visitor(Thread_Control *the_thread, void* arg)
> +{
> + if(the_thread->Start.Initial_stack.area == arg) {
> + shared_stack = &the_thread->the_stack;
> + return true;
> + }
ditto
> +}
> +#endif
>
> /**
> * mmap chain of mappings.
> @@ -50,7 +92,10 @@ void *mmap(
> bool map_private;
> bool is_shared_shm;
> int err;
> -
> +#if defined (USE_THREAD_STACK_PROTECTION)
> + uint32_t memory_flags;
> + uintptr_t shared_stack_address;
> +#endif
> map_fixed = (flags & MAP_FIXED) == MAP_FIXED;
> map_anonymous = (flags & MAP_ANON) == MAP_ANON;
> map_shared = (flags & MAP_SHARED) == MAP_SHARED;
> @@ -64,26 +109,47 @@ void *mmap(
> errno = EINVAL;
> return MAP_FAILED;
> }
> -
> +#if defined (USE_THREAD_STACK_PROTECTION)
> +/*
> + * We cannot share a part of the stack, hence, offset cannot be zer
> + */
> + if(off == 0) {
> + errno = EINVAL;
> + return MAP_FAILED;
> + }
> +#endif
> /*
> * We can provide read, write and execute because the memory in RTEMS does
> - * not normally have protections but we cannot hide access to memory.
> + * not normally have protections but we cannot hide access to memory. For
> + * thread-stack protection we can provide no-access option, but stacks are
> + * implicitly isolated and it makes no sense to specify no-access option for
> + * already isolated stacks.
> */
> if ( prot == PROT_NONE ) {
> errno = ENOTSUP;
> return MAP_FAILED;
> }
>
> +#if defined (USE_THREAD_STACK_PROTECTION)
> +/**
> + * MAP_ANONYMOUS, MAP_PRIVATE and MAP_FIXED are not supported for thread-stack protection.
> + * We can only have MAP_SHARED.
> +*/
> + if(map_anonymous || map_fixed || map_private || ~map_shared) {
It should be ! instead of ~ for logical complement instead of bitwise
complement.
> + errno = EINVAL;
> + return MAP_FAILED;
> + }
> +#else
> /*
> * We can not normally provide restriction of write access. Reject any
> * attempt to map without write permission, since we are not able to
> * prevent a write from succeeding.
> */
> +
Avoiding making random changes to existing code.
> if ( PROT_WRITE != (prot & PROT_WRITE) ) {
> errno = ENOTSUP;
> return MAP_FAILED;
> }
> -
> /*
> * Anonymous mappings must have file descriptor set to -1 and the offset
> * set to 0. Shared mappings are not supported with Anonymous mappings at
> @@ -93,7 +159,6 @@ void *mmap(
> errno = EINVAL;
> return MAP_FAILED;
> }
> -
> /*
> * If MAP_ANON is declared without MAP_PRIVATE or MAP_SHARED,
> * force MAP_PRIVATE
> @@ -122,7 +187,7 @@ void *mmap(
>
> /* Check for illegal addresses. Watch out for address wrap. */
> if ( map_fixed ) {
> - if ((uintptr_t)addr & PAGE_MASK) {
> + if ((uintptr_t)addr & PAGE_MASK) {
ditto
> errno = EINVAL;
> return MAP_FAILED;
> }
> @@ -185,7 +250,39 @@ void *mmap(
> return MAP_FAILED;
> }
> }
> +#endif
Maybe instead of carving up the existing code with these CPP blocks,
you should refactor to function calls.
> +#if defined ( USE_THREAD_STACK_PROTECTION )
> + memory_flags = mmap_flag_translate( prot );
>
> +/**
> + * We need to open a shared memory object for sharing stack.
> + */
> +
no blank, keep the comment close to the code it refers
> + if ( S_ISREG( sb.st_mode ) || S_ISBLK( sb.st_mode ) ||
> + S_ISCHR( sb.st_mode ) || S_ISFIFO( sb.st_mode ) ||
> + S_ISSOCK( sb.st_mode ) ) {
> + errno = EINVAL;
> + return MAP_FAILED;
> + }
> +
> + err = (*iop->pathinfo.handlers->mmap_h)(
> + iop, &shared_stack_address, len, prot, off );
break long function calls with 1 arg per line
https://docs.rtems.org/branches/master/eng/coding-80cols.html
I guess there isn't an explicit example of function calls there.
> +
> + if(err != 0) {
ws
> + return MAP_FAILED;
> + }
> +
> +/*
> + * We obtain the thread stack attributes of the target thread and the sharing
> + * thread, based on their addresses.
> + */
> + rtems_task_iterate(get_target_thread_visitor, addr);
> + rtems_task_iterate(get_shared_thread_visitor, shared_stack_address);
Pretty sure I said already that I don't think this is the right way to
solve this problem.
> +/*
> + * Share the stack address od the sharing thread with the target thread.
> + */
> + _Stackprotection_Share_stack(addr, shared_stack_address, len, memory_flags);
> +#else
> /* Create the mapping */
> mapping = malloc( sizeof( mmap_mapping ));
> if ( !mapping ) {
> @@ -299,4 +396,5 @@ void *mmap(
> mmap_mappings_lock_release( );
>
> return mapping->addr;
> -}
> +#endif
> +}
> \ No newline at end of file
> diff --git a/cpukit/posix/src/shmwkspace.c b/cpukit/posix/src/shmwkspace.c
> index 4fa4ec4771..aa46f000c0 100644
> --- a/cpukit/posix/src/shmwkspace.c
> +++ b/cpukit/posix/src/shmwkspace.c
> @@ -18,12 +18,35 @@
> #include <string.h>
> #include <rtems/score/wkspace.h>
> #include <rtems/posix/shmimpl.h>
> +#include <rtems/stackname.h>
> +
> +#define USE_THREAD_STACK_PROTECTION
>
> int _POSIX_Shm_Object_create_from_workspace(
> POSIX_Shm_Object *shm_obj,
> size_t size
> )
> {
> +#if defined(USE_THREAD_STACK_PROTECTION)
> + POSIX_Shm_Control *shm;
> +
> + shm = RTEMS_CONTAINER_OF(shm_obj, POSIX_Shm_Control, shm_object);
> + /** We assign fixed pattern of naming for thread-stacks, and treat them
> + * accordingly.
> + */
> + if( strncmp(shm->Object.name.name_p, "/taskfs/", 8) == 0 ) {
> + shm_obj->handle = rtems_stack_address_get(shm->Object.name.name_p);
> + shm_obj->size = size;
> + } else {
> + shm_obj->handle = _Workspace_Allocate( size );
> + if ( shm_obj->handle == NULL ) {
> + return ENOMEM;
> + }
> +
> + shm_obj->size = size;
> + return 0;
> +}
> +#else
> shm_obj->handle = _Workspace_Allocate( size );
> if ( shm_obj->handle == NULL ) {
> return ENOMEM;
> @@ -32,6 +55,7 @@ int _POSIX_Shm_Object_create_from_workspace(
> memset( shm_obj->handle, 0, size );
> shm_obj->size = size;
> return 0;
> +#endif
> }
>
> int _POSIX_Shm_Object_delete_from_workspace( POSIX_Shm_Object *shm_obj )
> diff --git a/cpukit/rtems/src/stackname.c b/cpukit/rtems/src/stackname.c
> new file mode 100644
> index 0000000000..d3d58bef53
> --- /dev/null
> +++ b/cpukit/rtems/src/stackname.c
> @@ -0,0 +1,102 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +
> +/**
> + * @file
> + *
> + * @ingroup
> + *
> + * @brief RTEMS Stack name
> + */
> +
> +/*
> + * 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.
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include "config.h"
> +#endif
> +
> +#include <string.h>
> +#include <rtems/score/objectimpl.h>
> +#include <rtems/score/thread.h>
> +
> +#define USE_THREAD_STACK_PRTOECTION
> +
> +/*
> + * Address of the required stack
> + */
> +void *stack_address;
> +
> +/*
> + * Name of the required stack
> + */
> +char *stack_name;
> +
> +static bool stack_address_get_visitor(Thread_Control *the_thread, void *arg)
> +{
> + char* name;
> +
> + name = arg;
> +
> +#if defined ( USE_THREAD_STACK_PROTECTION )
> + if(name != NULL) {
> + if ( strcmp(name, the_thread->the_stack.name) == 0 ) {
> + stack_address = the_thread->the_stack.Base.stack_address;
> + return true;
> + }
> + }
> +#endif
> +}
> +
> +static bool stack_name_get_visitor(Thread_Control *the_thread, void *arg)
> +{
> + void* address;
> +
> + address = arg;
> +
> +#if defined ( USE_THREAD_STACK_PROTECTION )
> + if( address != NULL) {
> + if ( address == the_thread->the_stack.Base.stack_address ) {
> + stack_name = the_thread->the_stack.name;
> + return true;
> + }
> + }
> +#endif
> +}
> +
> +void *rtems_stack_address_get( char* name )
> +{
> +#if defined (USE_THREAD_STACK_PROTECTION)
> + rtems_task_iterate( stack_name_set_visitor, name );
> +#endif
> + return stack_address;
> +}
> +
> +void *rtems_stack_name_get ( void* address )
> +{
> +#if defined ( USE_THREAD_STACK_PROTECTION )
> + rtems_task_iterate( stack_name_set_visitor, address );
> +#endif
> +return stack_name;
> +}
> \ No newline at end of file
> diff --git a/cpukit/score/cpu/arm/cpu.c b/cpukit/score/cpu/arm/cpu.c
> index 07b9588afd..58f2830087 100644
> --- a/cpukit/score/cpu/arm/cpu.c
> +++ b/cpukit/score/cpu/arm/cpu.c
> @@ -170,4 +170,4 @@ void _CPU_Initialize( void )
> /* Do nothing */
> }
>
> -#endif /* ARM_MULTILIB_ARCH_V4 */
> +#endif /* ARM_MULTILIB_ARCH_V4 */
?
> \ No newline at end of file
> diff --git a/cpukit/score/cpu/arm/cpu_asm.S b/cpukit/score/cpu/arm/cpu_asm.S
> index 66f8ba6032..21ae761c97 100644
> --- a/cpukit/score/cpu/arm/cpu_asm.S
> +++ b/cpukit/score/cpu/arm/cpu_asm.S
> @@ -66,6 +66,47 @@ DEFINE_FUNCTION_ARM(_CPU_Context_switch)
>
> str r3, [r0, #ARM_CONTEXT_CONTROL_ISR_DISPATCH_DISABLE]
>
> +#if defined ( USE_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 */
> + ldr r0, [r1, #112]
> + ldr r1, [r1, #116]
> + mov r2, #6
> + 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]
Does this mean the executing thread needs to have access permission to
the heir's stack?
> +/* Load the parameters of the function call */
> + ldr r1, [r0, #116]
> + ldr r0, [r0, #112]
> + bl _Memory_protection_Unset_entries
> + /*
> + * 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 +136,26 @@ DEFINE_FUNCTION_ARM(_CPU_Context_switch)
>
> /* Start restoring context */
> .L_restore:
> +#if defined ( USE_THREAD_STACK_PROTECTION )
> + cmp r1, r0
> + beq .L_memory_entries_set
> +
> +.L_memory_entries_set:
> +/* Save the registers modified during function call */
> + mov r8, r0
> + mov r9, r1
> + mov r10, r2
> +/* Load the parameters for function call */
> + ldr r0, [r1, #112]
> + ldr r1, [r1, #116]
> + mov r2, #6
> + bl _Memory_protection_Set_entries
> +
> +/* Restore the saved registers */
> + mov r0, r8
> + mov r1, r9
> + mov r2, r10
> +#endif
> #if !defined(RTEMS_SMP) && defined(ARM_MULTILIB_HAS_LOAD_STORE_EXCLUSIVE)
> clrex
> #endif
> @@ -133,6 +194,7 @@ DEFINE_FUNCTION_ARM(_CPU_Context_switch)
> */
> DEFINE_FUNCTION_ARM(_CPU_Context_restore)
> mov r1, r0
> +
> GET_SELF_CPU_CONTROL r2
> b .L_restore
>
> @@ -162,4 +224,4 @@ DEFINE_FUNCTION_ARM(_CPU_Context_restore)
> b .L_check_is_executing
> #endif
>
> -#endif /* ARM_MULTILIB_ARCH_V4 */
> +#endif /* ARM_MULTILIB_ARCH_V4 */
> \ No newline at end of file
> diff --git a/cpukit/score/cpu/arm/include/rtems/score/cpu.h b/cpukit/score/cpu/arm/include/rtems/score/cpu.h
> index b7b48a3ac3..214540b291 100644
> --- a/cpukit/score/cpu/arm/include/rtems/score/cpu.h
> +++ b/cpukit/score/cpu/arm/include/rtems/score/cpu.h
> @@ -669,4 +669,4 @@ typedef uintptr_t CPU_Uint32ptr;
>
> /** @} */
>
> -#endif /* _RTEMS_SCORE_CPU_H */
> +#endif /* _RTEMS_SCORE_CPU_H */
> \ No newline at end of file
> diff --git a/cpukit/score/src/stackprotection.c b/cpukit/score/src/stackprotection.c
> new file mode 100644
> index 0000000000..8ac0db96da
> --- /dev/null
> +++ b/cpukit/score/src/stackprotection.c
> @@ -0,0 +1,108 @@
> +/* 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.
> + */
> +
> +
1 blank line
> +#ifdef HAVE_CONFIG_H
> +#include "config.h"
> +#endif
> +
> +#include <rtems/score/stackprotection.h>
> +
> +void _Stackprotection_Context_switch(
> + Stackprotection_Stack *executing_stack,
> + Stackprotection_Stack *heir_stack
> +)
> +{
> +
> + Chain_Node *node;
> + Chain_Control *shared_node_control;
> + Stackprotection_Shared_stack *shared_stack;
> +
> + shared_node_control = &executing_stack->shared_stack_control;
> +
> + if( shared_node_control != NULL) {
> + node = _Chain_Get_first_unprotected( shared_node_control );
> + while( _Chain_Is_tail(shared_node_control, node) == false) {
> + shared_stack = RTEMS_CONTAINER_OF( node, Stackprotection_Shared_stack, node);
> + _Memory_protection_Unset_entries( shared_stack->Base.stack_address, shared_stack->Base.size );
> + }
> + }
> +
> + _Stackprotection_Context_restore( heir_stack );
> +}
> +
> +void _Stackprotection_Context_restore(
> + Stackprotection_Stack *heir_stack
> +)
> +{
> + Chain_Node *node;
> + uint32_t flags;
> + Chain_Control *shared_node_control;
> + Stackprotection_Shared_stack *shared_stack;
> +
> + shared_node_control = &heir_stack->shared_stack_control;
> +
> + if( shared_node_control != NULL) {
> + node = _Chain_Get_first_unprotected( shared_node_control );
> + while( _Chain_Is_tail(shared_node_control, node) == false) {
> + shared_stack = RTEMS_CONTAINER_OF( node, Stackprotection_Shared_stack, node);
> + flags = shared_stack->Base.access_flags;
> + _Memory_protection_Set_entries( shared_stack->Base.stack_address, shared_stack->Base.size, flags );
> + }
> + }
> +}
> +
> +void _Stackprotection_Share_stack(
> + Stackprotection_Stack *shared_stack,
> + Stackprotection_Stack *target_stack,
> + size_t shared_stack_size,
> + uint32_t flag
> +)
> +{
> +
> + Chain_Control *control;
> + void* shared_address;
> +
> + control = &target_stack->shared_stack_control;
> + shared_address = shared_stack->Base.stack_address;
> +
> + shared_stack->shared_stacks->Base.access_flags = flag;
> + shared_stack->shared_stacks->Base.size = shared_stack_size;
> + shared_stack->shared_stacks->Base.stack_address = shared_address;
> +
> + if( control != NULL) {
> + _Chain_Append_unprotected( control, &shared_stack->shared_stacks->node);
> + _Memory_protection_Set_entries(shared_address, shared_stack_size, flag);
> + }
> +}
We still haven't discussed/resolved the design of this switching logic.
> \ No newline at end of file
> diff --git a/cpukit/score/src/threadhandler.c b/cpukit/score/src/threadhandler.c
> index acbe186a53..ec873a6b5a 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>
>
> /*
> @@ -90,6 +91,11 @@ void _Thread_Handler( void )
> level = executing->Start.isr_level;
> _ISR_Set_level( level );
>
> + /*
> + * Restore the shared thread-stacks
> + */
> + _Stackprotection_Context_restore( &executing->the_stack );
> +
> /*
> * Initialize the floating point context because we do not come
> * through _Thread_Dispatch on our first invocation. So the normal
> diff --git a/cpukit/score/src/threadinitialize.c b/cpukit/score/src/threadinitialize.c
> index 691f56388e..7220e3cb5e 100644
> --- a/cpukit/score/src/threadinitialize.c
> +++ b/cpukit/score/src/threadinitialize.c
> @@ -21,11 +21,19 @@
> #include <rtems/score/threadimpl.h>
> #include <rtems/score/schedulerimpl.h>
> #include <rtems/score/stackimpl.h>
> +#include <rtems/score/stackprotection.h>
> #include <rtems/score/tls.h>
> #include <rtems/score/userextimpl.h>
> #include <rtems/score/watchdogimpl.h>
> #include <rtems/config.h>
>
> +#define USE_THREAD_STACK_PROTECTION
> +
> +#if defined ( USE_THREAD_STACK_PROTECTION )
> + #define STR( s ) #s
this is not necessary
> + #define STACK_ADDRESS_NAME( stack_address ) "/taskfs/"STR( stack_address )
> +#endif
> +
> bool _Thread_Initialize(
> Thread_Information *information,
> Thread_Control *the_thread,
> @@ -84,7 +92,7 @@ bool _Thread_Initialize(
>
> stack_area = config->stack_area;
> stack_size = config->stack_size;
> -
> +
> /* Allocate floating-point context in stack area */
> #if ( CPU_HARDWARE_FP == TRUE ) || ( CPU_SOFTWARE_FP == TRUE )
> if ( config->is_fp ) {
> @@ -113,6 +121,15 @@ bool _Thread_Initialize(
> stack_area,
> stack_size
> );
> +
> + /**
> + * Initialize the protected stack attributes.
> + */
> +
> + the_thread->the_stack.Base.stack_address = stack_area;
> + the_thread->the_stack.Base.size = stack_size;
> + the_thread->the_stack.Base.access_flags = ( READ_WRITE | MEMORY_CACHED );
> + the_thread->the_stack.name = STACK_ADDRESS_NAME( stack_area );
>
> /*
> * Get thread queue heads
> @@ -122,6 +139,7 @@ bool _Thread_Initialize(
> );
> _Thread_queue_Heads_initialize( the_thread->Wait.spare_heads );
>
> +#if defined ( USE_THREAD_STACK_PROTECTION )
> /*
> * General initialization
> */
> @@ -131,6 +149,7 @@ bool _Thread_Initialize(
> the_thread->Start.is_preemptible = config->is_preemptible;
> the_thread->Start.budget_algorithm = config->budget_algorithm;
> the_thread->Start.budget_callout = config->budget_callout;
> +#endif
>
> _Thread_Timer_initialize( &the_thread->Timer, cpu );
>
> @@ -280,4 +299,4 @@ failed:
> );
> _Stack_Free( the_thread->Start.allocated_stack );
> return false;
> -}
> +}
> \ No newline at end of file
> diff --git a/testsuites/psxtests/psx16/init.c b/testsuites/psxtests/psx16/init.c
> index 8e20e6d376..a53c82de32 100644
> --- a/testsuites/psxtests/psx16/init.c
> +++ b/testsuites/psxtests/psx16/init.c
> @@ -83,4 +83,4 @@ void *POSIX_Init(void *argument)
>
> #define CONFIGURE_INIT
> #include <rtems/confdefs.h>
> -/* end of file */
> +/* end of file */
> \ No newline at end of file
> diff --git a/testsuites/samples/hello/init.c b/testsuites/samples/hello/init.c
> index 34ded37c55..850722cbd4 100644
> --- a/testsuites/samples/hello/init.c
> +++ b/testsuites/samples/hello/init.c
> @@ -41,4 +41,4 @@ static rtems_task Init(
> #define CONFIGURE_INITIAL_EXTENSIONS RTEMS_TEST_INITIAL_EXTENSION
>
> #define CONFIGURE_INIT
> -#include <rtems/confdefs.h>
> +#include <rtems/confdefs.h>
> \ No newline at end of file
> diff --git a/testsuites/samples/thread_stack_protection/init.c b/testsuites/samples/thread_stack_protection/init.c
> new file mode 100644
> index 0000000000..76c7c25228
> --- /dev/null
> +++ b/testsuites/samples/thread_stack_protection/init.c
> @@ -0,0 +1,89 @@
> +#ifdef HAVE_CONFIG_H
> +#include "config.h"
> +#endif
> +
> +#include <rtems.h>
> +#include <tmacros.h>
> +#include <pthread.h>
> +#include <rtems/score/memoryprotection.h>
> +
> +const char rtems_test_name[] = " THREAD STACK PROTECTION ";
> +
> +void* Test_routine( void* arg )
> +{
> +
> +}
> +
> +void *POSIX_Init( void *argument )
> +{
> + 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;
> +
> + TEST_BEGIN();
> +
> + /*
> + * We set the stack size as 8Kb.
> + */
> + stack_size1 = 8192;
> + stack_size2 = 8192;
> +
> + /*
> + * 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, READ_ONLY | MEMORY_CACHED );
> + _Memory_protection_Set_entries( stack_addr2, stack_size2, READ_ONLY | MEMORY_CACHED );
> +
> + pthread_join( id1, NULL );
> + /*
> + * Write to the stack address of thread1 after it has been switched out.
> + */
> + memset( stack_addr1, 0, stack_size1 );
> +
> + pthread_join( id2, NULL );
> + /*
> + * Write to the stack address of thread2 after it has been switched out.
> + */
> + memset( stack_addr2, 0, stack_size2 );
> +
> +
> + 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
> +
> +#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..ee69dbe627
> --- /dev/null
> +++ b/testsuites/samples/thread_stack_protection/thread_stack_protection.scn
> @@ -0,0 +1,19 @@
> +
> +*** 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
> +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
> diff --git a/testsuites/samples/thread_stack_sharing/init.c b/testsuites/samples/thread_stack_sharing/init.c
> new file mode 100644
> index 0000000000..4ed9592c8e
> --- /dev/null
> +++ b/testsuites/samples/thread_stack_sharing/init.c
> @@ -0,0 +1,121 @@
> +#ifdef HAVE_CONFIG_H
> +#include "config.h"
> +#endif
> +
> +#include <rtems.h>
> +#include <tmacros.h>
> +#include <pthread.h>
> +#include <mman.h>
> +#include <rtems/stackname.h>
> +#include <sys/fcntl.h>
> +#include <rtems/score/memoryprotection.h>
> +
> +const char rtems_test_name[] = " THREAD STACK SHARING ";
> +
> +void* Test_routine( void* arg )
> +{
> +
> +}
> +
> +void *POSIX_Init( void *argument )
> +{
> + void *stack_addr1;
> + void *stack_addr2;
> + void* addr;
> + size_t stack_size1;
> + size_t stack_size2;
> + pthread_t id1;
> + pthread_t id2;
> + pthread_attr_t attr1;
> + pthread_attr_t attr2;
> + int fd;
> + char* name;
> +
> + TEST_BEGIN();
> +
> + /*
> + * We set the stack size as 8Kb.
> + */
> + stack_size1 = 8192;
> + stack_size2 = 8192;
> +
> + /*
> + * 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, READ_ONLY | MEMORY_CACHED );
> + _Memory_protection_Set_entries( stack_addr2, stack_size2, READ_ONLY | MEMORY_CACHED );
> +
> + /*
> + * Obtain the name of the stack-address to be shared for allocating a shared
> + * memory object.
> + */
> +
> + name = rtems_stack_name_get( stack_addr1 );
> +
> + /*
> + * Create a shared memory object of the stack we want to share with
> + * appropraite permissions. We share the stack with read and write permission
> + */
> + fd = shm_open( name, O_CREAT | O_RDWR, S_IRUSR | S_IWUSR );
> +
> + /*
> + * Truncate the size of the file to the size of the stack.
> + */
> + ftruncate( fd, stack_size1 );
> +
> + /*
> + * For sharing the stack we specify the address of the
> + * thread-stack we want to share with, the size of the shared stack,
> + * protection and access flags, file descriptor of the shared memory objcet
> + */
> + addr = mmap( stack_addr2, stack_size1, PROT_READ | PROT_WRITE, O_RDWR, fd, 0 );
> + rtems_test_assert( addr != NULL );
> +
> + pthread_join( id1, NULL );
> + /*
> + * Write to the stack address of thread1 after it has been switched out.
> + */
> + memset( stack_addr1, 0, stack_size1 );
> +
> + pthread_join( id2, NULL );
> + /*
> + * Write to the stack address of thread2 after it has been switched out.
> + */
> + memset( stack_addr2, 0, stack_size2 );
> +
> +
> + 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
> +
> +#include <rtems/confdefs.h>
> \ No newline at end of file
> --
> 2.17.1
>
More information about the devel
mailing list