Excellent point, Greg.

I think either nop or setting a security error bit (which would cause an error the next time the entry or the PMP was used) would be better than making the exception depend on the data that's written to the CSR.  Having an exception on CSR data makes for an annoying pipeline timing issue.

      Bill

I’ve been keeping track of it, and I’m pretty happy with it.

On 21/12/19 09:59, Andrew Waterman wrote:

On Fri, Dec 20, 2019 at 7:04 PM Greg Favor <gfavor@...
<mailto:gfavor@...>> wrote:

This all looks very good, except for one issue with item 3b: "Adding
a new PMP rule with pmpcfg.L and pmpcfg.X bits set fails with
Security exception."

RISC-V architecture - rather nicely - currently avoids having any
"register-operate" instructions that cause architectural exceptions
based on data-dependent execution (e.g. based on the value of its
register operands).  Currently all computational instructions and
CSR rd/wr instructions conform to this.  In contrast, item 3b
violates this and would represent the first and only case in
which implementations have to signal an exception on a
"register-operate" instruction at execution time (versus based on
what can be checked at decode time).


Although I still haven’t grokked the proposal, I can say I agree with
your line of reasoning. This use case doesn’t justify crossing the
Rubicon of data-dependent traps. Your proposed solution below sounds
more consistent with both the PMP spec and the broader philosophy.

Is it worth adding a generic rationale comment, for example around the
definition of WARL?

Paolo

Hello Greg and thanks for your feedback !

Στις 2019-12-21 05:04, Greg Favor έγραψε:

This all looks very good, except for one issue with item 3b: "Adding a
new PMP rule with pmpcfg.L and pmpcfg.X bits set fails with Security
exception."
RISC-V architecture - rather nicely - currently avoids having any
"register-operate" instructions that cause architectural exceptions
based on data-dependent execution (e.g. based on the value of its
register operands). Currently all computational instructions and CSR
rd/wr instructions conform to this. In contrast, item 3b violates
this and would represent the first and only case in which
implementations have to signal an exception on a "register-operate"
instruction at execution time (versus based on what can be checked at
decode time).
If people agree that this is undesirable, then it seems like the
suggested alternative or "fix" to this would be that the write to a
pmpcfg CSR write with pmpcfg.L and pmpcfg.X bits set, would not be
performed (i.e. the write is ignored and the register remains
unchanged). If desired, one could imagine things like also setting
some form of "security error" bit in the new mseccfg CSR.
Greg

Good point, I've updated the document so that writing pmpcfg.L and pmpcfg.X while MML is set is ignored. We could signal this in a different way but I don't think it's worth the complexity. Initially I thought that raising an exception will alert software about the illegal operation it tried to perform but it can simply read back the register to verify that.

Regards,
Nick

Hello Greg,

Στις 2019-12-22 03:12, Greg Favor έγραψε:

I notice another seeming issue, in the "rationale" for item 3a, where
it says:

The rule locking becomes part of the definition of an M-mode-only
rule, since when a rule is added on M mode, if not locked, can be
modified or removed. On the other hand, S/U modes can’t modify PMP
rules anyway so locking them doesn’t make sense.

First, it would be good for this rule locking behavior to be
explicitly included in item 3's description of the behavioral changes
when mseccfg.MML is set.

In the current PMP spec, the only way for a rule to be enforced on M-mode is for that rule to have the L bit set, hence being locked. This behavior doesn't change, what does change is that such rules are now enforced _only_ on M-mode and not also on S/U-modes. We do mention this on 3a:

"An M-mode-only rule is enforced on Machine mode and denied on Supervisor or User modes. It also remains locked so that any further modifications to the configuration or address registers are ignored until system reset."

Second, is the intent of MML=1 that no more M-mode-only rules can be
added, or that only the existing M-mode-only rules can't be modified?
The latter more corresponds to the current Lock behavior, but this
opens the door to being able to add a higher priority (lower numbered)
M-mode-only rule that overrides an existing locked rule (or an
existing non-locked S/U-mode-only rule). Similarly the door is open
to changing an S/U-mode-only rule into an M-mode-only rule and giving
M-mode access to what used to be an S/U-mode-only area or memory. In
essence all these "doors" enable the intent of this PMP enhancement
proposal to be circumvented.
If the answer is that no more M-mode-only rules can be added when
MML=1, then this is still worrisome or at least uncomfortable.
Attacked M-mode software could still change S/U-mode-only rules in
some way that enables some sort of subtle security attack. Or the
attacked M-mode software modifies M-mode-only memory, then writes a
new overriding rule that makes that memory now accessible to
S/U-modes. The nature of an actual attack is not clear, but it is
concerning that someone with enough ingenuity could maybe mount a
security attack through playing games with the PMP rules that are not
currently locked.
But then again, I guess this risk (arguably low or minimal) is
necessary to allow the flexibility for M-mode to dynamically manage
and change the non-M-mode-only rules during system operation. So I
can't really argue against "the answer is that no more M-mode-only
rules can be added when MML=1". And whatever risk there is could be
mitigated a bit by maybe (?) explicitly recommending that locked rules
should all occupy the lowest numbered PMP entries, ahead of any and
all non-locked rules.

The idea as mentioned on 3b is to prevent new executable M-Mode-only rules to be added. We can't do much about M-mode accessing S/U mode's memory, it can either add a higher priority M-Mode-only rule (however this will prevent S/U-mode from accessing that region and the software there will probably crash), use MPRV, or temporarily remove the S/U-mode-only rule that corresponds to that region and access it (M-mode can still access regions without a corresponding PMP rule). Trying to further restrict this will make things much more complicated on the software side, increasing the probability of security bugs, and will greatly reduce flexibility.

The goal of this proposal is to limit the attack surface in the same way SMEP/SMAP limit the attack surface on Supervisor mode. On S-mode one can still create new virtual memory mappings or remove the U bit from the PTE and circumvent SMAP/SMEP, but it's still a useful feature to have and relatively cheap. In our case execution prevention is much more strict since M-mode can't add any more executable regions for itself (3b), and can only execute code from marked regions (3c) so memory access prevention may be circumvented but memory execution prevention can't be circumvented and MEP is the real deal here, especially on M/U systems where the whole OS will be running on M-mode.

Also note that the attack vector we are after, as mentioned on Introduction/Threat model, includes an attacker that can leverage a bug on M-mode to trick it and make it execute code from an attacker-controlled S/U memory region. We can't do much if an attacker has fully compromised M-mode in a way that he/she can tweak with PMP rules or set CSRs in general, in the same way we can't do much if an attacker has such control on S-mode where he/she can tweak with the page table. In such a scenario it's game over.

Regards,
Nick

Good point, Allen.  I think it would make good sense to begin assigning mCause values >= 64 where there's no likelihood of wanting a rapid delegation to a privilege below M.  The Security Exception is an obvious choice.

      Bill

On 1/7/20 10:43 PM, Bill Huffman wrote:

Good point, Allen.  I think it would make good sense to begin assigning
mCause values >= 64 where there's no likelihood of wanting a rapid
delegation to a privilege below M.  The Security Exception is an obvious
choice.

      Bill

ACK, I'll update the doc accordingly.

Regards,
Nick

Hello all,

I've been studying the TEE Task Group's "PMP Enhancements" proposal
with great interest off-and-on for several weeks. I definitely agree
with the intention of the proposal, but I see several issues. For
presentation, I've numbered them 1 through 4 below.

1.
Currently, when a memory access is prevented by physical memory
attributes (PMAs) or by PMP, an access fault trap is taken. The
proposal defines a new "security exception" and requires that some
blocked memory accesses take a security exception trap instead of the
usual access fault. The document says

A new exception will help distinguish the exceptions we get with
the current PMP spec when the access type doesn’t match R/W/X flags
on the matching rule, from the exception we get when violating the
access controls of the new mechanisms in place.

I request that some explanation be provided for how this distinction
is expected be helpful; i.e., why "denied" accesses need a different
exception code than "enforced" accesses. If the reason is supposed
to be obvious, it was not so to me. Just saying it "will help
distinguish" isn't sufficient. Why distinguish? What good does it do?

For me, it seems obvious that these should all be access faults.

2.
As Jonathan Behrens has already noted, some systems depend on being
able to set mstatus.MXR = 1 temporarily to read S/U-executable
instructions, for emulation purposes. The proposal should be modified
to say that any S/U-mode-only PMP region that grants execute permission
to S/U modes (bit X is set), implicitly grants read permission to
M mode when MXR = 1.

3.
I'm concerned that the use of the reserved combination W = 1, R = 0
for shared memory regions may be incompatible with a future use for
this encoding in page tables. For example, one possible allocation of
the reserved W/R encoding in page tables could be:

X W R

0 1 0 uncached read-only page
1 1 0 uncached read-write page

If so defined, the same _uncached_ property might also be sensible
for PMP entries, yet we would no longer be able to encode it the same
way, because we have allocated the reserved W/R combination for shared
memory regions instead.

To be clear, I know of no current plans to use the reserved W/R
encoding for an _uncached_ property this way, or for any other purpose.
I am merely giving an example of the sort of inconsistency that could
arise because of our choices today.

I understand that the reserved W/R encoding ended up in use because
there is opposition to touching the two reserved bits that still exist
in a PMP configuration byte, and there are few options for encoding
everything in just the four bits we already have: L, X, W, R. My own
choice would be to go ahead and consume a reserved bit to avoid the
risk of creating a mess of the encoding down the road.

4.
The biggest concern I have with the proposal is that the effort to
fully lock down the executable regions for M mode, while correct for
maximizing security in principle, doesn't leave enough flexibility
for some systems. Tariq Kurd has given an example of a system that,
during booting, progressively expands the regions accessible to M mode,
which the current proposal prohibits. I'd like to give a couple other
examples that are more specifically about execute permission, but
still revolve around the need to edit M-mode-only PMP entries even when
enhanced security is enabled.

Consider a complex operating system, running in M mode, that supports
loadable "kernel modules", which are components that can be brought
into memory or evicted in response to the varying needs of user-level
tasks. With the current PMP proposal, when MML = 1, this M-mode OS
cannot dynamically adjust the regions of memory that are executable for
loadable modules. Instead, the OS authors must make a choice: either
pre-allocate the maximal amount of memory that could ever be needed for
loaded kernel modules, possibly wasting memory, or entirely forgo using
the security enhancement. If they choose the latter because memory
really is scarce, how has security been improved?

Or consider the situation where there is more than one independent
stage of boot-time software that could benefit from enhanced PMP
security. U-Boot, for example, is a complex piece of software in its
own right. If a bootloader like U-Boot is used in an M/U-only system,
it's easy to imagine that enhanced security could help guard against
attacks. But with the current proposal, U-Boot cannot set MML = 1,
because it would have to configure the executable regions not only
for itself but also for the operating system it subsequently loads,
something outside its knowledge or authority. Because all current and
future executable regions must be known and configured before MML can
be enabled, a U-Boot-like loader must run with MML = 0. Again, this
seems like a loss for security in this instance.

I have no argument with anyone who needs all the restrictions the
current proposal provides; we should be able to offer that. But if we
require always that all executable regions be locked down in advance,
we're not providing sufficient flexibility for all systems at all
times, instead sometimes forcing an awkward "maximal security or none"
choice.

(To help his particular system, Mr. Kurd has proposed a DPL bit, Delay
PMP Lock. However, this bit would conflict with one of the intended
purposes of PMP locking as I understand it, which is to permit earlier
initialization software to protect some regions from access by later,
less trusted, M-mode code. By itself, the DPL solution is too simple
because, in general, we need to be able to set some PMP entries that
stay locked, while at the same time other entries remain unlocked for
editing but are nonetheless enforced.)

To bridge the gap between "maximal security" and "none", I've developed
a modified proposal with four security levels rather than just the
current two (MML = 0 or 1). Unfortunately, I see no good way to
provide all the needed flexibility without also taking one of the two
reserved PMP configuration bits. While having four security levels may
sound more complex, actually it's not, because the extra configuration
bit allows some encoding complexity to be reduced at the same time.
The only significant cost is the allocation of the reserved bit. I'll
be sending my modified proposal in a follow-up message.

Regards,

- John Hauser

I wrote:

2.
As Jonathan Behrens has already noted, some systems depend on being
able to set mstatus.MXR = 1 temporarily to read S/U-executable
instructions, for emulation purposes. The proposal should be modified
to say that any S/U-mode-only PMP region that grants execute permission
to S/U modes (bit X is set), implicitly grants read permission to
M mode when MXR = 1.

Correction: I believe that should say "... implicitly grants read
permission to S/U modes when in M mode and MXR = 1". This is relevant
only when MPRV = 1 and MPP = 0 or 1, so it's a rather narrow case.
Hopefully I've got it right this time.

- John Hauser

For 4..

As we discussed in the meeting we agree with the use case, lock is existing in current PMP.

The current PMP enhancement proposal tries to be back compatible and thus does not try to address this issue.

As promised, here is my suggestion for modifying the TEE Task Group's
"PMP Enhancements" proposal to have four security levels, to support a
larger set of use cases.

Instead of the two bits MML and MMLA, I propose a single two-bit field,
MSL (Memory Security Level) to control how PMP is interpreted. At
reset, MSL = 0, which causes PMP to act as originally standardized. If
higher security levels are supported, then all values 0 through 3 are
valid for MSL. When MSL = 3, my proposal provides security equal to
the task group's proposal. Values 1 and 2 choose intermediate security
levels between 0 and 3. Between resets, software may increase MSL, but
the hardware prevents decreases except by reset.

I there is also an independent DMC bit (Default Memory Closed). When
DMC = 1, if a memory access is attempted for which no PMP entries match,
an access fault occurs, whereas when DMC = 0, if no PMP entries match
for a memory access, accessibility is determined by the privilege mode
and MSL value. Reset sets DMC = 0. Software may set DMC = 1, but once
set, the hardware prevents clearing DMC except by reset.

In PMP configuration bytes, my proposal expands the single L bit into
a two-bit PL field (Privilege/Locked) in bits 7:6. For each PMP entry,
the PL field and global MSL value together determine the behavior as
follows:

When MSL = 0:

PL | Locked? | M mode | S/U mode

0 | No | All accesses allowed | Apply XWR bits
2 | Yes | Apply XWR bits | Apply XWR bits
default | All accesses allowed | No accesses allowed

At level 0, PMP acts as originally standardized. PL fields cannot be
set to 1 or 3 at this level. Writing a configuration byte with PL = 1
or 3 leaves PL = 0 or 2, respectively.

The "default" line in this and other tables indicates the accessibility
when no PMP entries match and DMC = 0. No accesses are allowed by
default when DMC = 1, of course.

When MSL = 1:

PL | Locked? | M mode | S/U mode

0 | No | Read/write allowed | Apply XWR bits
1 | No | No accesses allowed | Apply XWR bits
2 | Yes | Apply XWR bits | No accesses allowed
3 | No | Apply XWR bits | No accesses allowed
default | All accesses allowed | No accesses allowed

Compared to level 0, entries with PL = 1 or 3 are now supported, and
the permissions of PMP entries with PL = 0 or 2 are more restricted.

When MSL = 2:

PL | Locked? | M mode | S/U mode

0 | No | Read/write allowed | Apply XWR bits
1 | No | No accesses allowed | Apply XWR bits
2 | Yes | Apply XWR bits | No accesses allowed
3 | No | Apply XWR bits | No accesses allowed
default | Read/write allowed | No accesses allowed

The only difference from level 1 is that execute permission is no
longer included in the default for M mode when DMC = 0. (If DMC = 1,
levels 1 and 2 are the same.)

When MSL = 3:

PL | Locked? | M mode | S/U mode

0 | No | Read/write allowed | Apply XWR bits
1 | No | No accesses allowed | Apply XWR bits
2 | Yes | Apply XWR bits | No accesses allowed
3 | Yes | Apply XWR bits | No accesses allowed
default | Read/write allowed | No accesses allowed

In addition, level 3 prevents the configuration of new locked PMP
entries that give execute permission to M mode (PL = 2 or 3, and
bit X = 1). An attempt to configure such an entry is ignored. This
restriction, and the fact that PMP entries with PL = 3 are now locked
(making them identical to ones with PL = 2), are the only differences
between levels 2 and 3.

With this scheme, the most useful levels of enhanced security are
expected to be 2 and 3. As noted, level 3 provides maximal security
equivalent to the task group's proposal. Level 2 is available as a
fallback when level 3 is too constraining. Level 2 keeps the door open
for software to edit M-mode-only PMP entries (those with PL = 3) while
providing as much security as possible under the circumstances.

Level 1 may seem unnecessary, but it's actually a useful stepping stone
to get from level 0 to levels 2 or 3. It also provides a "pretty good"
intermediate level of protection that is less trouble for software and
may require fewer PMP entries, since it grants execute permission by
default while higher levels do not. Even if you believe that makes
level 1 too compromised, please consider that it's a huge step up from
level 0, and there may be those unwilling or unable (e.g., insufficient
number of PMP entries) to raise the security level to 2.

As an example use, the design I've presented supports Tariq Kurd's
system as follows: Shortly after reset, initialization software would
first set MSL = 1, then create an executable PMP entry with PL = 3 to
cover the initialization code itself, and lastly set DMC = 1. After
that preparation, multiple stages of boot software would be free to
create and edit PMP entries with PL = 3 to expand and contract the
regions that are accessible and/or executable. At the end of booting,
the final M-mode configurations can be permanently locked by raising
MSL to 3 before entering the operating state.

Regards,

- John Hauser

I wrote:

I there is also an independent DMC bit (Default Memory Closed).

"I assume there is also an independent DMC bit (Default Memory
Closed)."

- John Hauser