diff mbox series

[15/15] nvic: Implement "user accesses BusFault" SCS region behaviour

Message ID 1501692241-23310-16-git-send-email-peter.maydell@linaro.org
State Superseded
Headers show
Series v7M: cleanups and bugfixes prior to v8M | expand

Commit Message

Peter Maydell Aug. 2, 2017, 4:44 p.m. UTC 
The ARMv7M architecture specifies that most of the addresses in the
PPB region (which includes the NVIC, systick and system registers)
are not accessible to unprivileged accesses, which should
BusFault with a few exceptions:
 * the STIR is configurably user-accessible
 * the ITM (which we don't implement at all) is always
   user-accessible

Implement this by switching the register access functions
to the _with_attrs scheme that lets us distinguish user
mode accesses.

This allows us to pull the handling of the CCR.USERSETMPEND
flag up to the level where we can make it generate a BusFault
as it should for non-permitted accesses.

Note that until the core ARM CPU code implements turning
MEMTX_ERROR into a BusFault the registers will continue to
act as RAZ/WI to user accesses.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

---
 hw/intc/armv7m_nvic.c | 58 ++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 41 insertions(+), 17 deletions(-)

-- 
2.7.4

Comments

Edgar E. Iglesias Aug. 3, 2017, 3:59 p.m. UTC | #1 
On Wed, Aug 02, 2017 at 05:44:01PM +0100, Peter Maydell wrote:
> The ARMv7M architecture specifies that most of the addresses in the

> PPB region (which includes the NVIC, systick and system registers)

> are not accessible to unprivileged accesses, which should

> BusFault with a few exceptions:

>  * the STIR is configurably user-accessible

>  * the ITM (which we don't implement at all) is always

>    user-accessible

> 

> Implement this by switching the register access functions

> to the _with_attrs scheme that lets us distinguish user

> mode accesses.

> 

> This allows us to pull the handling of the CCR.USERSETMPEND

> flag up to the level where we can make it generate a BusFault

> as it should for non-permitted accesses.

> 

> Note that until the core ARM CPU code implements turning

> MEMTX_ERROR into a BusFault the registers will continue to

> act as RAZ/WI to user accesses.

> 

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>


Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



> ---

>  hw/intc/armv7m_nvic.c | 58 ++++++++++++++++++++++++++++++++++++---------------

>  1 file changed, 41 insertions(+), 17 deletions(-)

> 

> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c

> index 5a18025..bbfe2d5 100644

> --- a/hw/intc/armv7m_nvic.c

> +++ b/hw/intc/armv7m_nvic.c

> @@ -733,11 +733,8 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value)

>      }

>      case 0xf00: /* Software Triggered Interrupt Register */

>      {

> -        /* user mode can only write to STIR if CCR.USERSETMPEND permits it */

>          int excnum = (value & 0x1ff) + NVIC_FIRST_IRQ;

> -        if (excnum < s->num_irq &&

> -            (arm_current_el(&cpu->env) ||

> -             (cpu->env.v7m.ccr & R_V7M_CCR_USERSETMPEND_MASK))) {

> +        if (excnum < s->num_irq) {

>              armv7m_nvic_set_pending(s, excnum);

>          }

>          break;

> @@ -748,14 +745,32 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value)

>      }

>  }

>  

> -static uint64_t nvic_sysreg_read(void *opaque, hwaddr addr,

> -                                 unsigned size)

> +static bool nvic_user_access_ok(NVICState *s, hwaddr offset)

> +{

> +    /* Return true if unprivileged access to this register is permitted. */

> +    switch (offset) {

> +    case 0xf00: /* STIR: accessible only if CCR.USERSETMPEND permits */

> +        return s->cpu->env.v7m.ccr & R_V7M_CCR_USERSETMPEND_MASK;

> +    default:

> +        /* All other user accesses cause a BusFault unconditionally */

> +        return false;

> +    }

> +}

> +

> +static MemTxResult nvic_sysreg_read(void *opaque, hwaddr addr,

> +                                    uint64_t *data, unsigned size,

> +                                    MemTxAttrs attrs)

>  {

>      NVICState *s = (NVICState *)opaque;

>      uint32_t offset = addr;

>      unsigned i, startvec, end;

>      uint32_t val;

>  

> +    if (attrs.user && !nvic_user_access_ok(s, addr)) {

> +        /* Generate BusFault for unprivileged accesses */

> +        return MEMTX_ERROR;

> +    }

> +

>      switch (offset) {

>      /* reads of set and clear both return the status */

>      case 0x100 ... 0x13f: /* NVIC Set enable */

> @@ -826,11 +841,13 @@ static uint64_t nvic_sysreg_read(void *opaque, hwaddr addr,

>      }

>  

>      trace_nvic_sysreg_read(addr, val, size);

> -    return val;

> +    *data = val;

> +    return MEMTX_OK;

>  }

>  

> -static void nvic_sysreg_write(void *opaque, hwaddr addr,

> -                              uint64_t value, unsigned size)

> +static MemTxResult nvic_sysreg_write(void *opaque, hwaddr addr,

> +                                     uint64_t value, unsigned size,

> +                                     MemTxAttrs attrs)

>  {

>      NVICState *s = (NVICState *)opaque;

>      uint32_t offset = addr;

> @@ -839,6 +856,11 @@ static void nvic_sysreg_write(void *opaque, hwaddr addr,

>  

>      trace_nvic_sysreg_write(addr, value, size);

>  

> +    if (attrs.user && !nvic_user_access_ok(s, addr)) {

> +        /* Generate BusFault for unprivileged accesses */

> +        return MEMTX_ERROR;

> +    }

> +

>      switch (offset) {

>      case 0x100 ... 0x13f: /* NVIC Set enable */

>          offset += 0x80;

> @@ -853,7 +875,7 @@ static void nvic_sysreg_write(void *opaque, hwaddr addr,

>              }

>          }

>          nvic_irq_update(s);

> -        return;

> +        return MEMTX_OK;

>      case 0x200 ... 0x23f: /* NVIC Set pend */

>          /* the special logic in armv7m_nvic_set_pending()

>           * is not needed since IRQs are never escalated

> @@ -870,9 +892,9 @@ static void nvic_sysreg_write(void *opaque, hwaddr addr,

>              }

>          }

>          nvic_irq_update(s);

> -        return;

> +        return MEMTX_OK;

>      case 0x300 ... 0x33f: /* NVIC Active */

> -        return; /* R/O */

> +        return MEMTX_OK; /* R/O */

>      case 0x400 ... 0x5ef: /* NVIC Priority */

>          startvec = 8 * (offset - 0x400) + NVIC_FIRST_IRQ; /* vector # */

>  

> @@ -880,26 +902,28 @@ static void nvic_sysreg_write(void *opaque, hwaddr addr,

>              set_prio(s, startvec + i, (value >> (i * 8)) & 0xff);

>          }

>          nvic_irq_update(s);

> -        return;

> +        return MEMTX_OK;

>      case 0xd18 ... 0xd23: /* System Handler Priority.  */

>          for (i = 0; i < size; i++) {

>              unsigned hdlidx = (offset - 0xd14) + i;

>              set_prio(s, hdlidx, (value >> (i * 8)) & 0xff);

>          }

>          nvic_irq_update(s);

> -        return;

> +        return MEMTX_OK;

>      }

>      if (size == 4) {

>          nvic_writel(s, offset, value);

> -        return;

> +        return MEMTX_OK;

>      }

>      qemu_log_mask(LOG_GUEST_ERROR,

>                    "NVIC: Bad write of size %d at offset 0x%x\n", size, offset);

> +    /* This is UNPREDICTABLE; treat as RAZ/WI */

> +    return MEMTX_OK;

>  }

>  

>  static const MemoryRegionOps nvic_sysreg_ops = {

> -    .read = nvic_sysreg_read,

> -    .write = nvic_sysreg_write,

> +    .read_with_attrs = nvic_sysreg_read,

> +    .write_with_attrs = nvic_sysreg_write,

>      .endianness = DEVICE_NATIVE_ENDIAN,

>  };

>  

> -- 

> 2.7.4

> 

>
Richard Henderson Aug. 3, 2017, 10:23 p.m. UTC | #2 
On 08/02/2017 09:44 AM, Peter Maydell wrote:
> The ARMv7M architecture specifies that most of the addresses in the

> PPB region (which includes the NVIC, systick and system registers)

> are not accessible to unprivileged accesses, which should

> BusFault with a few exceptions:

>  * the STIR is configurably user-accessible

>  * the ITM (which we don't implement at all) is always

>    user-accessible

> 

> Implement this by switching the register access functions

> to the _with_attrs scheme that lets us distinguish user

> mode accesses.

> 

> This allows us to pull the handling of the CCR.USERSETMPEND

> flag up to the level where we can make it generate a BusFault

> as it should for non-permitted accesses.

> 

> Note that until the core ARM CPU code implements turning

> MEMTX_ERROR into a BusFault the registers will continue to

> act as RAZ/WI to user accesses.

> 

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

> ---

>  hw/intc/armv7m_nvic.c | 58 ++++++++++++++++++++++++++++++++++++---------------

>  1 file changed, 41 insertions(+), 17 deletions(-)


Reviewed-by: Richard Henderson <richard.henderson@linaro.org>



r~
diff mbox series

Patch

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 5a18025..bbfe2d5 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -733,11 +733,8 @@  static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value)
     }
     case 0xf00: /* Software Triggered Interrupt Register */
     {
-        /* user mode can only write to STIR if CCR.USERSETMPEND permits it */
         int excnum = (value & 0x1ff) + NVIC_FIRST_IRQ;
-        if (excnum < s->num_irq &&
-            (arm_current_el(&cpu->env) ||
-             (cpu->env.v7m.ccr & R_V7M_CCR_USERSETMPEND_MASK))) {
+        if (excnum < s->num_irq) {
             armv7m_nvic_set_pending(s, excnum);
         }
         break;
@@ -748,14 +745,32 @@  static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value)
     }
 }
 
-static uint64_t nvic_sysreg_read(void *opaque, hwaddr addr,
-                                 unsigned size)
+static bool nvic_user_access_ok(NVICState *s, hwaddr offset)
+{
+    /* Return true if unprivileged access to this register is permitted. */
+    switch (offset) {
+    case 0xf00: /* STIR: accessible only if CCR.USERSETMPEND permits */
+        return s->cpu->env.v7m.ccr & R_V7M_CCR_USERSETMPEND_MASK;
+    default:
+        /* All other user accesses cause a BusFault unconditionally */
+        return false;
+    }
+}
+
+static MemTxResult nvic_sysreg_read(void *opaque, hwaddr addr,
+                                    uint64_t *data, unsigned size,
+                                    MemTxAttrs attrs)
 {
     NVICState *s = (NVICState *)opaque;
     uint32_t offset = addr;
     unsigned i, startvec, end;
     uint32_t val;
 
+    if (attrs.user && !nvic_user_access_ok(s, addr)) {
+        /* Generate BusFault for unprivileged accesses */
+        return MEMTX_ERROR;
+    }
+
     switch (offset) {
     /* reads of set and clear both return the status */
     case 0x100 ... 0x13f: /* NVIC Set enable */
@@ -826,11 +841,13 @@  static uint64_t nvic_sysreg_read(void *opaque, hwaddr addr,
     }
 
     trace_nvic_sysreg_read(addr, val, size);
-    return val;
+    *data = val;
+    return MEMTX_OK;
 }
 
-static void nvic_sysreg_write(void *opaque, hwaddr addr,
-                              uint64_t value, unsigned size)
+static MemTxResult nvic_sysreg_write(void *opaque, hwaddr addr,
+                                     uint64_t value, unsigned size,
+                                     MemTxAttrs attrs)
 {
     NVICState *s = (NVICState *)opaque;
     uint32_t offset = addr;
@@ -839,6 +856,11 @@  static void nvic_sysreg_write(void *opaque, hwaddr addr,
 
     trace_nvic_sysreg_write(addr, value, size);
 
+    if (attrs.user && !nvic_user_access_ok(s, addr)) {
+        /* Generate BusFault for unprivileged accesses */
+        return MEMTX_ERROR;
+    }
+
     switch (offset) {
     case 0x100 ... 0x13f: /* NVIC Set enable */
         offset += 0x80;
@@ -853,7 +875,7 @@  static void nvic_sysreg_write(void *opaque, hwaddr addr,
             }
         }
         nvic_irq_update(s);
-        return;
+        return MEMTX_OK;
     case 0x200 ... 0x23f: /* NVIC Set pend */
         /* the special logic in armv7m_nvic_set_pending()
          * is not needed since IRQs are never escalated
@@ -870,9 +892,9 @@  static void nvic_sysreg_write(void *opaque, hwaddr addr,
             }
         }
         nvic_irq_update(s);
-        return;
+        return MEMTX_OK;
     case 0x300 ... 0x33f: /* NVIC Active */
-        return; /* R/O */
+        return MEMTX_OK; /* R/O */
     case 0x400 ... 0x5ef: /* NVIC Priority */
         startvec = 8 * (offset - 0x400) + NVIC_FIRST_IRQ; /* vector # */
 
@@ -880,26 +902,28 @@  static void nvic_sysreg_write(void *opaque, hwaddr addr,
             set_prio(s, startvec + i, (value >> (i * 8)) & 0xff);
         }
         nvic_irq_update(s);
-        return;
+        return MEMTX_OK;
     case 0xd18 ... 0xd23: /* System Handler Priority.  */
         for (i = 0; i < size; i++) {
             unsigned hdlidx = (offset - 0xd14) + i;
             set_prio(s, hdlidx, (value >> (i * 8)) & 0xff);
         }
         nvic_irq_update(s);
-        return;
+        return MEMTX_OK;
     }
     if (size == 4) {
         nvic_writel(s, offset, value);
-        return;
+        return MEMTX_OK;
     }
     qemu_log_mask(LOG_GUEST_ERROR,
                   "NVIC: Bad write of size %d at offset 0x%x\n", size, offset);
+    /* This is UNPREDICTABLE; treat as RAZ/WI */
+    return MEMTX_OK;
 }
 
 static const MemoryRegionOps nvic_sysreg_ops = {
-    .read = nvic_sysreg_read,
-    .write = nvic_sysreg_write,
+    .read_with_attrs = nvic_sysreg_read,
+    .write_with_attrs = nvic_sysreg_write,
     .endianness = DEVICE_NATIVE_ENDIAN,
 };