[PATCH v2] Strict thread-stack isolation
Utkarsh Rai
utkarsh.rai60 at gmail.com
Fri Jul 17 17:49:48 UTC 2020
On Fri, Jul 17, 2020 at 11:08 AM Gedare Bloom <gedare at 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.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?
>
>
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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20200717/4a0ad60f/attachment-0001.html>
More information about the devel
mailing list