[PATCH v2] Strict thread-stack isolation
Gedare Bloom
gedare at rtems.org
Fri Jul 17 05:38:10 UTC 2020
Is this code working? how do you test it?
There are several points buried below that you might need to bring out
to separate threads for discussion, while you work on the code.
On Thu, Jul 16, 2020 at 5:34 AM Utkarsh Rai <utkarsh.rai60 at gmail.com> wrote:
>
> - This is the complete set of changes for strict isolation of thread stacks.
> - There needs to be a confiuration operation,(#if defined(USE_THREAD_STACK_PROTECTION) for simple configuration can be used)
> - The stack attributes are allocated through malloc, this needs to be done through score unlimited objects.
>
> Mechanism for thread-stack isolation
> - Whenever a stack is allocated we assign READ_WRITE memory attributes to the memory region, the stack attribute structure is appended to a chain for tracking
> - On each context switch, the executing stack is marked as 'not-current' and we unset its memory attributes. The heir stack is marked as 'current'
> - On context restore we set the memory attributes of the heir stack and iterate thorugh the chain to check for any other 'current' stack and unset its memory
> attribute (This requires some refinement, so that we don't have to iterate over the chain).
Commit formatting:
https://docs.rtems.org/branches/master/eng/vc-users.html#creating-a-patch
points to: https://devel.rtems.org/wiki/Developer/Git#GitCommits
-- that should probably get merged to the docs.
It may not say in our docs, but wrap your commit messages to 80 chars.
The first line looks best at about 60 characters when it gets used as
a 'summary'. The way you've written a set of bullets is more like
paragraphs. You might just aim to write paragraphs. Bullets are nice
when you want to format a list of related items. In that case, you
should provide a list heading for some structure/organization.
> ---
> .../include/bsp/arm-cp15-set-ttb-entries.h | 0
> bsps/arm/xilinx-zynq/mmu/bsp-set-mmu-attr.c | 79 ++++++++
> bsps/shared/start/stackalloc.c | 16 ++
> c/src/lib/libbsp/arm/xilinx-zynq/Makefile.am | 5 +-
> cpukit/Makefile.am | 1 +
> cpukit/headers.am | 2 +
> cpukit/include/rtems/score/memorymanagement.h | 89 +++++++++
> cpukit/include/rtems/score/stackprotection.h | 149 ++++++++++++++
> cpukit/score/cpu/arm/cpu.c | 4 +
> cpukit/score/cpu/arm/cpu_asm.S | 43 +++-
> .../score/cpu/arm/include/rtems/score/cpu.h | 18 ++
> cpukit/score/src/stackprotection.c | 185 ++++++++++++++++++
> 12 files changed, 589 insertions(+), 2 deletions(-)
> create mode 100644 bsps/arm/include/bsp/arm-cp15-set-ttb-entries.h
> create mode 100644 bsps/arm/xilinx-zynq/mmu/bsp-set-mmu-attr.c
> create mode 100644 cpukit/include/rtems/score/memorymanagement.h
> create mode 100644 cpukit/include/rtems/score/stackprotection.h
> create mode 100644 cpukit/score/src/stackprotection.c
>
> diff --git a/bsps/arm/include/bsp/arm-cp15-set-ttb-entries.h b/bsps/arm/include/bsp/arm-cp15-set-ttb-entries.h
> new file mode 100644
> index 0000000000..e69de29bb2
> diff --git a/bsps/arm/xilinx-zynq/mmu/bsp-set-mmu-attr.c b/bsps/arm/xilinx-zynq/mmu/bsp-set-mmu-attr.c
> new file mode 100644
> index 0000000000..0c82f113a9
> --- /dev/null
> +++ b/bsps/arm/xilinx-zynq/mmu/bsp-set-mmu-attr.c
> @@ -0,0 +1,79 @@
> +#include <bsp/arm-cp15-start.h>
> +#include <rtems/score/memorymanagement.h>
> +#include <libcpu/arm-cp15.h>
> +#include <rtems.h>
> +
This could use a comment why it is necessary. And also a note about
converting to a configuration option
> +#ifdef USE_THREAD_STACK_PROTECTION
> + #define ARM_MMU_USE_SMALL_PAGES
> +#endif
> +
Although it is not strictly required to have doxygen in the BSPs, I
would like to see some function documentation of this new interface.
> +void Memorymanagement_Set_entries(uintptr_t begin, size_t size, Memorymanagement_flags flags)
I don't like the namespace choice. Did you discuss this with anyone?
The naming rules are in
https://docs.rtems.org/branches/master/eng/coding-naming.html
I guess we haven't codified the BSP interface rules. You might inquire
in a separate thread for some discussion. From what I recall, we have
bsp_* for some generic kind of functions that should be implemented by
bsp. So if this is meant to be a BSP implemented interface, you might
like "bsp_memory_management" or some such.
Memory management might be an overloaded term that could encompass
allocation/deallocation, sharing, protection, translation/virtual
memory, maybe more. If allocation is not in scope, then some other
name might be better. I guess you may be inspired by MMU. If the
point of this interface is to manipulate the mmu/mpu hardware then the
name should reflect that. I know we've had these discussions in the
past, every time a student works on this topic--did you look for/find
any prior discussion or interface descriptions?
Do we really need a typedef for the flags? Are they ever more than a
bit-mask? I'm just not convinced it can't be simply: uint32_t flags;
> +{
> +
no blank line needed after {
> + uintptr_t end;
> + rtems_interrupt_level irq_level;
> + uint32_t access_flags;
> +
> + end = begin + size;
> + access_flags = Memorymanagement_Translate_flags(flags);
Maybe it is better for users to get the flags in the right format ahead of time?
> +
> + /**
> + * The ARM reference manual instructs to disable all the interrupts before
> + * setting up page table entries.
> + */
I still have questions/concerns about whether the PTEs are shared
across multiple cores. If so, then interrupt disable is insufficient
if this function can be called in parallel by multiple threads.
Also, (why) doesn't arm_cp15_set_translation_table_entries() provide
protection itself, internally, before modifying the PTEs? That may be
worth a separate discussion.
> + rtems_interrupt_disable(irq_level);
> + arm_cp15_set_translation_table_entries(begin, end, access_flags);
> + rtems_interrupt_enable(irq_level);
> +}
> +
> +void Memorymanagement_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 = Memorymanagement_Translate_flags(NO_ACCESS);
white space inside function parameters, like: ...Translate_flags( NO_ACCES );
> +
> + /**
> + * 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);
> +}
> +
> +
Never need to put 2 blank lines in a row.
> +uint32_t Memorymanagement_Translate_flags(Memorymanagement_flags attr_flags)
Is this function meant to be callable from outside of the above two
functions? If not, it can be made 'private' (static) in this file.
> +{
> + uint32_t flags;
> + switch (attr_flags)
> + {
Put { on same line as the conditional expression (switch statement).
https://docs.rtems.org/branches/master/eng/coding-conventions.html#formatting
"Put braces on the same line as and one space after the conditional
expression ends."
> + case READ_WRITE:
Indent case statements within the scoped block. (For some reason, vim
doesn't like to indent them correctly for you by default.) So here
there should be four blank spaces since the case statement is at the
second level of scope.
> + flags = ARMV7_MMU_READ_WRITE;
> + break;
indent/align the break statement with the rest of the code in the case
> +
> + case READ_WRITE_CACHED:
> + flags = ARMV7_MMU_DATA_READ_WRITE_CACHED;
> + break;
> +
> + case READ_ONLY:
> + flags = ARMV7_MMU_READ_ONLY;
> + break;
> +
> + case READ_ONLY_CACHED:
> + flags = ARMV7_MMU_READ_ONLY_CACHED;
> + break;
> +
> + case NO_ACCESS:
> + flags = 0;
> + break;
> +
> + default:
> + return 0;
> + break;
> + }
> +
> + return flags;
> +}
> \ No newline at end of file
> diff --git a/bsps/shared/start/stackalloc.c b/bsps/shared/start/stackalloc.c
> index f7cf7be0f1..ea8028c8db 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/stackprotection.h>
>
> #include <bsp/linker-symbols.h>
>
> @@ -43,6 +44,7 @@ void bsp_stack_allocate_init(size_t stack_space_size)
> void *bsp_stack_allocate(size_t size)
> {
> void *stack = NULL;
> + uintptr_t page_table_base;
>
> if (bsp_stack_heap.area_begin != 0) {
> stack = _Heap_Allocate(&bsp_stack_heap, size);
> @@ -52,6 +54,20 @@ void *bsp_stack_allocate(size_t size)
> stack = _Workspace_Allocate(size);
> }
>
> +#ifdef USE_THREAD_STACK_PROTECTION
> + /**
> + * Although we are not performing page table switching, still we assign a value
> + * to avoid compiler warniing.
> + */
> + page_table_base = (uintptr_t)0x1000;
There's (still) stray whitespace at the end of the line.
You can suppress unused variable warnings by using this code line:
(void) page_table_base;
> +
> + /**
> + * The current way to get protected stack is to assign memory attributes
> + * to the allocated memory.
> + */
I don't really understand this comment.
> + _Stackprotection_Allocate_attr( (uintptr_t)stack, size, page_table_base );
> +
> +#endif
> return stack;
> }
>
> diff --git a/c/src/lib/libbsp/arm/xilinx-zynq/Makefile.am b/c/src/lib/libbsp/arm/xilinx-zynq/Makefile.am
> index cfd59475c2..490f99792e 100644
> --- a/c/src/lib/libbsp/arm/xilinx-zynq/Makefile.am
> +++ b/c/src/lib/libbsp/arm/xilinx-zynq/Makefile.am
> @@ -74,6 +74,9 @@ librtemsbsp_a_SOURCES += ../../../../../../bsps/arm/xilinx-zynq/i2c/cadence-i2c.
> # Cache
> librtemsbsp_a_SOURCES += ../../../../../../bsps/arm/shared/cache/cache-l2c-310.c
>
> +#MMU
> +librtemsbsp_a_SOURCES += ../../../../../../bsps/arm/xilinx-zynq/mmu/bsp-set-mmu-attr.c
> +
> # Start hooks
> librtemsbsp_a_SOURCES += ../../../../../../bsps/arm/xilinx-zynq/start/bspstarthooks.c
> librtemsbsp_a_SOURCES += ../../../../../../bsps/arm/xilinx-zynq/start/bspstartmmu.c
> @@ -85,4 +88,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..83f9bfb3ef 100644
> --- a/cpukit/Makefile.am
> +++ b/cpukit/Makefile.am
> @@ -929,6 +929,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..2f16c71d9c 100644
> --- a/cpukit/headers.am
> +++ b/cpukit/headers.am
> @@ -352,6 +352,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/memorymanagement.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 +406,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/memorymanagement.h b/cpukit/include/rtems/score/memorymanagement.h
> new file mode 100644
> index 0000000000..2a5490c680
> --- /dev/null
> +++ b/cpukit/include/rtems/score/memorymanagement.h
> @@ -0,0 +1,89 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +
> +/**
> + * @file
> + *
> + * @ingroup RTEMSScoreMemorymanagement
> + *
> + * @brief This file provodes APIs for high-level memory management
typo: provides
> + *
> + */
> +
> +/*
> + * 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_MEMORYMANAGEMENT_H
> +#define _RTEMS_SCORE_MEMORYMANAGEMENT_H
> +
> +#include <rtems/score/basedefs.h>
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +/** This defines the various high-level memory access flags.*/
> +typedef enum
> +{
> + READ_WRITE,
> + READ_WRITE_CACHED,
> + READ_ONLY,
> + READ_ONLY_CACHED,
> + NO_ACCESS
> +} Memorymanagement_flags;
> +
> +/**
> + * @brief Define the memory access permission for the specified memory region
> + *
> + * @param begin_addr Begining of the memory region
> + * @param size Size of the memory region
> + * @param flag Memory access flag
> + *
> + */
> +void Memorymanagement_Set_entries(uintptr_t begin_addr, size_t size, Memorymanagement_flags flag);
I definitely don't like this name in the score. It needs to be
reconsidered. Note that (although it is reserved) we use a leading
underscore character for functions in the supercore.
I might suggest _Memory_protection_Xxx_xxx.
Again, these interfaces have been discussed in the past, and some
prior GSoC proposals had some sketches as well.
> +
> +/**
> + * @brief Unset the memory access permission for the specified memory region
> + * This operation implcitly sets the specified memory region with 'NO_ACCESS'
typo: implicitly
> + * flag.
> + *
> + * @param begin_addr Begining of the memory region
typo 'Beginning'
> + * @param size Size of the memory region
> + */
> +void Memorymanagement_Unset_entries(uintptr_t begin_addr, size_t size);
> +
> +/**
> + * @brief Translate the high-level flags to BSP-specifc MMU flags.
typo: specific
Please edit your code a bit more carefully for typos.
> + *
> + * @param attr_flags High-level memory access flags.
> + *
> + * @retval flag BSP-specifc MMU flag
> + */
> +uint32_t Memorymanagement_Translate_flags(Memorymanagement_flags attr_flags);
> +
> +#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..91e3f6daba
> --- /dev/null
> +++ b/cpukit/include/rtems/score/stackprotection.h
> @@ -0,0 +1,149 @@
> +/**
> + * @file
> + *
> + * @ingroup RTEMSScoreStackprotection
> + *
> + * @brief Stackprotection API
space
> + *
> + * This include file provides the API for the management of protected thread-
> + * stacks
> + */
I'm not convinced this interface is necessary or the right way to do
this. Why not just integrate directly with threads/thread stacks?
Or is there a reasonable way to accomplish this by using the custom
Stack Allocator, maybe in combination with a context switch hook
call-out? I know we discussed this, but I can't remember the
resolution. and as I look at this, it just looks a little bit overly
complicated for what it is trying to get done. At least for this
specific purpose of stack protection.
> +
> +/*
> + * 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/memorymanagement.h>
> + #include <rtems/score/chainimpl.h>
> +#endif
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#if !defined ( ASM )
> +
> +/**
> + * The following defines the attributes of a protected stack
> + */
> +typedef struct
> +{
> + /** This is the node to the chain for tracking each allocated/shared stack */
> + Chain_Node node;
> + /** This is the stack address */
> + uintptr_t stack_address;
> + /** This is the stack size */
> + size_t size;
> + /** This is the pointer to the page table base */
> + uintptr_t page_table_base;
> + /**Memory flag for the alllocated/shared stack */
> + Memorymanagement_flags access_flags;
> +} Stackprotection_Attr;
> +
> +/**
> + * The following defines the control block of a shared stack
> + */
> +typedef struct
> +{
> + /** This is the attribute of a shared stack*/
> + Stackprotection_Attr Base;
> +} Stackprotection_Shared_stack;
> +
> +/**
> + * The following defines the control block of an allocated stack
> + */
> +typedef struct
> +{
> + /** This is the attribute of an allocated stack*/
> + Stackprotection_Attr Base;
> + /** This is the pointer to the attributes of a stack shared with the stack
> + * in question
> + */
> + Stackprotection_Shared_stack *shared_stacks;
> + /**This marks if the stack in question belongs to an executing thread*/
> + bool current_stack;
> +} Stackprotection_The_stack;
Stack_protection_Stack would be better. "_The_stack" is a bit odd.
> +
> +/**
> + * @brief Allocate the attributes of the stack in question.
> + *
> + * @param freechain The stack address.
freechain?
> + * @param size Size of the stack.
> + * @param page_table_base Pointer to the start of a page table
> + */
> +void _Stackprotection_Allocate_attr(uintptr_t stack_address, size_t size, uintptr_t page_table_base);
> +
> +/**
> + * @brief Share a stack with another stack.
> + *
> + * @param shared_stack The stack to be shared
> + * @param target_stack The stack with which to share
> + */
> +void _Stackprotection_Share_stack(Stackprotection_The_stack *shared_stack, Stackprotection_The_stack* target_stack);
> +
> +/**
> + * @brief Initialize the context of a thread with the control block of a
> + * protected stack
> + *
> + * @retval the_stack The protected stack
> + */
> +Stackprotection_The_stack *_Stackprotection_Context_initialize(void);
> +
> +/**
> + * @brief Swap out the executing protected 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_The_stack *executing_stack, Stackprotection_The_stack *heir_stack);
> +
> +/**
> + * @brief Swap the restored protected stack 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_The_stack *restored_stack);
> +
> +#endif /* !defined ( ASM ) */
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif
> diff --git a/cpukit/score/cpu/arm/cpu.c b/cpukit/score/cpu/arm/cpu.c
> index 07b9588afd..cbb62d45e5 100644
> --- a/cpukit/score/cpu/arm/cpu.c
> +++ b/cpukit/score/cpu/arm/cpu.c
> @@ -28,6 +28,7 @@
>
> #include <rtems/score/assert.h>
> #include <rtems/score/cpu.h>
> +#include <rtems/score/stackprotection.h>
> #include <rtems/score/thread.h>
> #include <rtems/score/tls.h>
>
> @@ -97,6 +98,9 @@ void _CPU_Context_Initialize(
> {
> (void) new_level;
>
> + #if defined (USE_THREAD_STACK_PROTECTION)
> + the_context->the_stack = _Stackprotection_Context_initialize();
> + #endif
align #endif with the #if. And we discussed recently about whether or
not to indent them. I can't remember if there was a resolution, but
generally they are not indented at all.
> the_context->register_sp = (uint32_t) stack_area_begin + stack_area_size;
> the_context->register_lr = (uint32_t) entry_point;
> the_context->isr_dispatch_disable = 0;
> diff --git a/cpukit/score/cpu/arm/cpu_asm.S b/cpukit/score/cpu/arm/cpu_asm.S
> index 66f8ba6032..d4d98af5b2 100644
> --- a/cpukit/score/cpu/arm/cpu_asm.S
> +++ b/cpukit/score/cpu/arm/cpu_asm.S
> @@ -66,6 +66,27 @@ DEFINE_FUNCTION_ARM(_CPU_Context_switch)
>
> str r3, [r0, #ARM_CONTEXT_CONTROL_ISR_DISPATCH_DISABLE]
>
> +#if defined ( USE_THREAD_STACK_PROTECTION )
> + /*
> + * Save the registers modifed during function call
> + */
> + mov r8, r0
> + mov r9, r1
> + mov r10, r2
I already commented about some of this code before, but the space
alignment here is inconsistent
> + /*
> + * Load the parameters to be passed
> + */
> + ldr r0, [r0, #ARM_CONTEXT_CONTROL_STACK_ATTR_OFFSET]
> + ldr r1, [r1, #ARM_CONTEXT_CONTROL_STACK_ATTR_OFFSET]
> + bl _Stackprotection_Context_switch
And I'm not sure it is great to add a function call here. Especially
considering that, currently, you're just switching out one active
stack (executing) for another (heir), this could be done directly,
albeit with more ASM written but less runtime overhead.
> + /*
> + * 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
> @@ -132,7 +153,27 @@ DEFINE_FUNCTION_ARM(_CPU_Context_switch)
> *
> */
> DEFINE_FUNCTION_ARM(_CPU_Context_restore)
> - mov r1, r0
> +
> + mov r1, r0
avoid spurious formatting changes to surrounding code
> +#if defined( USE_THREAD_STACK_PROTECTION )
> + /*
> + * Save the registers modifed during function call
> + */
> + mov r8, r0
> + mov r9, r1
> + mov r10, r2
> + /*
> + * Load the parameters to be passed
> + */
> + ldr r0, [r0, #ARM_CONTEXT_CONTROL_STACK_ATTR_OFFSET]
> + bl _Stackprotection_Context_switch
I'm not sure why you call the same function in save as restore. Also,
r1 is not set to anything?
> + /*
> + * Restore the saved registers
> + */
> + mov r0, r8
> + mov r1, r9
> + mov r2, r10
> +#endif
> GET_SELF_CPU_CONTROL r2
> b .L_restore
>
> diff --git a/cpukit/score/cpu/arm/include/rtems/score/cpu.h b/cpukit/score/cpu/arm/include/rtems/score/cpu.h
> index b7b48a3ac3..df13fed258 100644
> --- a/cpukit/score/cpu/arm/include/rtems/score/cpu.h
> +++ b/cpukit/score/cpu/arm/include/rtems/score/cpu.h
> @@ -34,6 +34,7 @@
> #include <rtems/score/paravirt.h>
> #endif
> #include <rtems/score/arm.h>
> +#include <rtems/score/stackprotection.h>
>
> /**
> * @addtogroup RTEMSScoreCPUARM
> @@ -157,6 +158,21 @@
>
> #ifdef ARM_MULTILIB_HAS_THREAD_ID_REGISTER
> #define ARM_CONTEXT_CONTROL_THREAD_ID_OFFSET 44
> + #ifdef ARM_MULITLIB_VFP
> + #define ARM_STACK_PROT_ATTR_OFFSET 112
> + #else
> + #define ARM_STACK_PROT_ATTR_OFFSET 48
> + #endif
> +#endif
> +
> +#ifdef USE_THREAD_STACK_PROTECTION
> + #if defined ARM_MULITLIB_VFP
> + #define ARM_CONTEXT_CONTROL_STACK_ATTR_OFFSET 112
> + #elif ARM_MULTILIB_HAS_THREAD_ID_REGISTER
> + #define ARM_CONTEXT_CONTROL_STACK_ATTR_OFFSET 48
> + #else
> + #define ARM_CONTEXT_CONTROL_STACK_ATTR_OFFSET 44
> + #endif
> #endif
>
> #ifdef ARM_MULTILIB_VFP
> @@ -185,6 +201,7 @@
>
> #define ARM_VFP_CONTEXT_SIZE 264
>
> +
remove stray changes
> #ifndef ASM
>
> #ifdef __cplusplus
> @@ -235,6 +252,7 @@ typedef struct {
> #ifdef RTEMS_SMP
> volatile bool is_executing;
> #endif
> +Stackprotection_The_stack *the_stack;
I don't see that this needs to be in the context structure. It is more
logically part of the task control block. (I suppose it makes it
easier for you to get the pointer, but that is just a minor
convenience. You can also pass the pointer to the TCB, or offset from
it.)
> } Context_Control;
>
> static inline void _ARM_Data_memory_barrier( void )
> diff --git a/cpukit/score/src/stackprotection.c b/cpukit/score/src/stackprotection.c
> new file mode 100644
> index 0000000000..7f7c3fd6e5
> --- /dev/null
> +++ b/cpukit/score/src/stackprotection.c
> @@ -0,0 +1,185 @@
> +/* 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.
> + */
> +
> +
2 lines
> +#ifdef HAVE_CONFIG_H
> +#include "config.h"
> +#endif
> +
> +#include <rtems/score/stackprotection.h>
> +
> +Chain_Control _Stackprotection_node_control = CHAIN_INITIALIZER_EMPTY(_Stackprotection_node_control);
> +
> +static void _Stackprotection_Remove_prev_entry(void)
> +{
> +
> + Chain_Node *node;
> + Stackprotection_The_stack *prot_stack;
> +
> + if( _Chain_Is_empty(&_Stackprotection_node_control) == true ) {
add a space after if.
if ( _Chain...
> + _Chain_Initialize_empty(&_Stackprotection_node_control);
> + }
> + node = _Chain_First( &_Stackprotection_node_control );
> +
> + while(_Chain_Is_tail(&_Stackprotection_node_control, node) == false) {
ditto for while
> +
> + prot_stack = RTEMS_CONTAINER_OF(node,Stackprotection_The_stack, Base.node);
> +
> + if( prot_stack->current_stack == false ) {
> + Memorymanagement_Unset_entries(prot_stack->Base.stack_address, prot_stack->Base.size);
> + }
> + node = _Chain_Immutable_next( node );
> + }
> +
I think I already complained about this design. Please discuss and
justify the need for a dynamic data structure instead of a single
pointer, or even an array of pointers.
> +}
> +
> +/*
> +Iterate to the end of the chain and mark all the 'currnet' stack as false
> +Append the current stack attribute to the end of the chain
> +*/
> +static void _Stackprotection_Append_chain (Chain_Control *control, Stackprotection_The_stack *stack_append_attr)
> +{
> + Chain_Node *node;
> + Stackprotection_The_stack *present_stacks_attr;
> +
> + if(_Chain_Is_empty(&_Stackprotection_node_control) == true ) {
> +
the blank line here does not help readability. Use blank lines to
separate semi- or un-related chunks of code.
> + _Chain_Initialize_one(&_Stackprotection_node_control, &stack_append_attr->Base.node);
> + } else {
> + _Chain_Append_unprotected(&_Stackprotection_node_control, &stack_append_attr->Base.node);
> + }
> +}
> +
> +void _Stackprotection_Allocate_attr(uintptr_t stack_address, size_t size, uintptr_t page_table_base)
> +{
> + Stackprotection_The_stack *prot_stack;
> +
> +/*This field will be refactored and score objects will be used for dynamic allocation*/
> + prot_stack = malloc(sizeof(Stackprotection_The_stack));
Or, this stuff can be included in the TCB or stack structures that
already exist and are already managed by the Object Allocation. You
could add the extra fields contingent on the configuration option for
thread stack protection.
> +
> + if(prot_stack != NULL) {
> + prot_stack->Base.stack_address = stack_address;
> + prot_stack->Base.size = size;
> + prot_stack->Base.page_table_base = page_table_base;
> + prot_stack->Base.access_flags = READ_WRITE_CACHED;
> + prot_stack->current_stack = true;
wrong indent level
> + }
> +
> + /*
> + Add the attribute field to the end of the chain, remove the memory entries of
> + previously allocated stack and set the memory entry of the currnet stack.
> + */
> + _Stackprotection_Append_chain(&_Stackprotection_node_control, prot_stack );
> + Memorymanagement_Set_entries(stack_address, size, READ_WRITE_CACHED);
> +
> +}
> +
> +Stackprotection_The_stack *_Stackprotection_Context_initialize(void)
> +{
> + Chain_Node *node;
> + Stackprotection_The_stack *prot_stack;
> +
> + if( _Chain_Is_empty(&_Stackprotection_node_control) == false ) {
> + node = _Chain_First( &_Stackprotection_node_control );
> +
> + while( _Chain_Is_tail(&_Stackprotection_node_control, node ) == false) {
> + prot_stack = RTEMS_CONTAINER_OF( node, Stackprotection_The_stack, Base.node);
> +
> + if(prot_stack->current_stack == true) {
> + return prot_stack;
> + } else {
> + node = _Chain_Immutable_next( node );
> + }
> + }
> + }
> +
> + return prot_stack;
> +}
> +
> +void _Stackprotection_Context_switch(Stackprotection_The_stack *executing_stack, Stackprotection_The_stack *heir_stack)
> +{
> + void *stack_address;
> + size_t size;
> + Chain_Node *node;
> + Chain_Control *shared_node_control;
> +
> + /*
> + Remove the stacks shared with the current stack by iterating the chain
> + */
> + if( executing_stack != NULL) {
> +
> + stack_address = executing_stack->Base.stack_address;
> + size = executing_stack->Base.size;
> +
> + if(executing_stack->current_stack == true) {
> + executing_stack->current_stack = false;
> + Memorymanagement_Unset_entries(stack_address, size);
> + }
> + }
> +
> + _Stackprotection_Context_restore(heir_stack);
> +
> +}
> +
> +void _Stackprotection_Context_restore(Stackprotection_The_stack *heir_stack)
> +{
> + void *stack_address;
> + size_t size;
> + Chain_Node *node;
> + Memorymanagement_flags flags;
> + Chain_Control *shared_node_control;
> + Stackprotection_The_stack *present_stacks_attr;
> +
> + if(heir_stack != NULL) {
> + heir_stack->current_stack = true;
too much indent
> + stack_address = heir_stack->Base.stack_address;
> + size = heir_stack->Base.size;
> + flags = heir_stack->Base.access_flags;
> + Memorymanagement_Set_entries(stack_address, size, flags);
> + }
> +
> + node = _Chain_First(&_Stackprotection_node_control);
> + /** Iterate through the chain and unset memory entries of all the
> + * previous thread-stacks
> + */
> + while(_Chain_Is_tail(&_Stackprotection_node_control,node) == false) {
whitespace:
while ( _Chain_Is_tail( &_...,node ) == false ) {
> +
> + present_stacks_attr = RTEMS_CONTAINER_OF(node, Stackprotection_The_stack, Base.node);
> +
> + if(present_stacks_attr->current_stack == true && present_stacks_attr != heir_stack)
> + present_stacks_attr->current_stack = false;
> + Memorymanagement_Unset_entries(present_stacks_attr->Base.stack_address, present_stacks_attr->Base.size);
> + node = _Chain_Immutable_next( node );
> + }
> +}
> --
> 2.17.1
>
More information about the devel
mailing list