mirage / mirage-crypto

Cryptographic primitives for OCaml, in OCaml (also used in MirageOS)
ISC License
77 stars 43 forks source link

riscv64: Replace rdcycle64 with rdtime64 #194

Closed AdrianBunk closed 9 months ago

AdrianBunk commented 9 months ago

rdcycle is privileged since kernel 6.6, resulting in SIGILL: https://buildd.debian.org/status/fetch.php?pkg=ocaml-mirage-crypto&arch=riscv64&ver=0.11.2-1%2Bb3&stamp=1708088363&raw=0

hannesm commented 9 months ago

Thanks for your PR - @edwintorok since you contributed the original code, would approve this change?

edwintorok commented 9 months ago

The standard doesn't specify the accuracy of rdtime, and since access to rdcycle was restricted for "security" reasons it'll likely be lower resolution than rdcycle. I haven't measured myself, but according to https://zhangruiyi.me/files/riscv_attacks_sp23.pdf rdcycle gives you 1ns accuracy, whereas rdtime gives you 45ns, but it could be even lower resolution on other platforms/implementations.

So if we replace rdcycle by rdtime I think we no longer meet the criteria for mc_cycle_counter which says that every call should lead to a different output, we'll probably have to maintain our own counter and add that to the value read by rdtime to ensure uniqueness.

edwintorok commented 9 months ago

It might be better to use rdcycle in unikernel mode (where I assume you'd have access to this instruction because you run at a sufficient privilege level), and rdtime in userspace mode (where it is better to rely on /dev/urandom anyway, very few RISC-V CPUs are out-of-order yet, and on an in-order CPU I doubt the whirlwind loop would introduce much entropy beyond the value of the counter itself, but IIUC the cycle counter is used just as an additional source of entropy,and not the only source, so that is probably fine).

Perhaps use #if defined(__ocaml_freestanding__) || defined(__ocaml_solo5__) and use rdcycle there as before (I don't know whether mirage supports RISC-V yet, but when it does this code will be ready), and rdtime otherwise?

hannesm commented 9 months ago

Thanks @edwintorok -- what Edwin said makes a lot of sense to me, I added code comments that implement his suggestion. Would be great if they could be integrated and tested (I suspect the rdcycle code path is at the moment dead code, since I haven't seen a MirageOS unikernel on RISC-V (yet)).

hannesm commented 9 months ago

Could you @AdrianBunk (or @edwintorok) -- or someone else with access to RISC-V hardware please embed the suggestions above and report whether this compiles fine (or not)?

hannesm commented 9 months ago

another friendly ping, we're close to a release of mirage-crypto, and it would be great to include this... but without any testing on risc-v (or an approval by e.g. @edwintorok with the suggested changes above), I don't feel comfortable to merge and release.

edwintorok commented 9 months ago

I can attempt to do a test this weekend on at least one of my RISC-V boards.