On Fri, Jul 17, 2020 at 11:08 AM Gedare Bloom <ged...@rtems.org> wrote:
> 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.ra...@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? > > As you suggested earlier, maybe this can be modeled along the lines of the cache manager? Maybe 'rtems_memory_protection_xx_xx' is a good naming pattern (This will be moved to cpukit/include/rtems)? > 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? > Agreed, this way we won't have 'Memorymanagement_Translate_flags' in the public interface. > > + > > + /** > > + * 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. > > Ok, I will enquire about this separately. > > + 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. > > We discussed that the way to handle the dynamic allocation of stack attributes was through score objects. I have had this in my to-do notes and I will integrate the protected stack structure with the thread control block. Nevertheless, we need methods to manage these protected stacks (Context switching/ restoration/ initialization, stack sharing ) this interface provides some of these methods as well as a structure to handle the attributes of the protected/shared stacks. There are some simplifications that I have figured out in the current implementation. > + > > +/* > > + * 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. Every time a new thread is allocated we need to allocate its corresponding stack-protection control block, just like we allocate TCB for each thread. I realize this will make more sense if we integrate the structure with the TCB. > > +} > > + > > +/* > > +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 > > >
_______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel