Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

John Hauser public review comments #134

Merged
merged 1 commit into from
Oct 22, 2021
Merged

John Hauser public review comments #134

merged 1 commit into from
Oct 22, 2021

Conversation

mjosaarinen
Copy link
Collaborator

Implements the changes proposed by John Hauser during the public review (16-Oct-2021), related to virtual (VS and VU) modes. Additional minor modifications;

  • The modes M/S/U/VS/VU/HS are typeset with normal upper case font (not monospace), for consistency with the privileged specification.
  • Also for consistency with that specification, the names of exceptions (virtual instruction exception and illegal instruction exception) are not capitalized.
  • Some minor changes, such as a reference to SP 800-90B Section 3.1.5.1.1 instead of 3.1.5.1.2 for vetted conditioners, and moving some paragraphs around.
Markku-Juhani Saarinen   Oct 16, 2021, 8:43:33 AM 
to John Hauser, RISC-V ISA Dev

On Sat, Oct 16, 2021 at 1:41 AM John Hauser <[email protected]> wrote:
>  I have some concerns about the proposed extension Zkr, Entropy Source
>  Extension, which is a part of the Scalar Cryptography extensions,
>  described in _RISC-V Cryptography Extensions Volume I: Scalar & Entropy
>  Source Instructions_.


Hi John,

Thanks for the further review! I personally think that your suggestions make sense.

I've itemized the proposed changes below as :
- JH-R1: (Non-func.) Terminology, user mode -> unprivileged (Sect. 4.1),
- JH-R2: (Functional) Entropy source is never available in VS/VU (Sect 4.3 and parts of Appendix B).
- JH-R3: (Functional) Use of "virtual instruction exception" to implement [JH-R2] for VS/VU (Table in Sect 4.3).
- JH-R4: (Non-func.) Terminology, related to HS mode (Sect. 4.3)


> The first is a minor point:  Section 4.1 says "seed is a user mode
> CSR".  In fact, seed should be labeled as an _unprivileged_ CSR, not
> user-mode.  The difference is that the seed CSR may exist even if
> U mode isn't implemented.

[JH-R1] Change of wording to unprivileged; makes sense to me. As noted elsewhere, "seed" can definitely be available also in simple systems that implement M-mode only.
 
> The more significant issues involve Section 4.3, "Access Control to
> seed".

> The mseccfg bits sseed and useed that grant access to CSR seed from
> S and U modes must not grant access also from VS and VU modes.  The
> simplest fix is to always prohibit access from VS and VU modes.  If
> desired, a hypervisor can emulate accesses to the seed CSR from a
> virtual machine.


[JH-R2] Denial of access from VS and VU modes; makes sense to me. The ("virtual source") emulation is specifically appropriate for VS and VU modes and - due to security concerns - and we can always prevent direct access.

My rationale: In my (admittedly limited) hypervisor understanding the VS and VU modes are generally less trusted. Direct access to the entropy source implies the ability to deny it to others (depletion), along with other effects, as discussed in (non-normative) Appendix B.3.5. The S-mode access is intended only for "simple" systems for performance reasons; e.g. single-instance Linux-like system where the system kernel can can be trusted for this purpose.

> When a trap occurs due to an attempt to access seed from VS or
> VU mode, the specific exception should depend on the type of access
> and the value of the sseed bit of mseccfg.  If the attempted access
> is read-only (never allowed), an illegal instruction exception must be
>  raised.  Else, following the rules for virtual instruction exceptions
> laid down by the hypervisor extension, if mseccfg.sseed = 1, a virtual
> instruction exception should be raised, and if mseccfg.sseed = 0, an
> illegal instruction exception should be raised.


[JH-R3]   Modifying the table to include "virtual instruction exception" for VS and VU modes in the cases where access would currently be granted; this seems like a sensible way to implement [JH-R2] and I can imagine that it makes exception handling easier. (I will read up on "virtual instruction exception" to have the correct references.)


> Lastly, HS mode is described as an "additional mode", but it really
> should be thought of as an expansion of S mode, not a new mode.


[JH-R4]  I'm sure you're right. We can change the language appropriately.


Cheers,
- markku

Dr. Markku-Juhani O. Saarinen <[email protected]> PQShield, Oxford UK.

@ben-marshall ben-marshall merged commit c4ec443 into riscv:master Oct 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants