[RISC-V] [tech-tee] TEE proposal for M-mode SMEP/SMAP via PMP


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


On Fri, Dec 20, 2019 at 3:23 PM Andrew Waterman <andrew@...> wrote:
PrivArch folks,

The TEE TG has proposed additional PMP functionality to mitigate some forms of security attack against M-mode.  A description of the threat model and the proposed augmentation is at the link below.  Please review and provide feedback on this thread; I'll do the same shortly.


Thanks,
Andrew


Andrew Waterman
 



On Fri, Dec 20, 2019 at 7:04 PM Greg Favor <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.


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


On Fri, Dec 20, 2019 at 3:23 PM Andrew Waterman <andrew@...> wrote:
PrivArch folks,

The TEE TG has proposed additional PMP functionality to mitigate some forms of security attack against M-mode.  A description of the threat model and the proposed augmentation is at the link below.  Please review and provide feedback on this thread; I'll do the same shortly.


Thanks,
Andrew


Paolo Bonzini
 

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


Nick Kossifidis
 

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