xiaoxiang781216 commented on pull request #2497: URL: https://github.com/apache/incubator-nuttx/pull/2497#issuecomment-742705823
> > at least, three libc(glibc, musl and bionic) implement getrandom: > > https://github.com/bminor/musl/blob/master/include/sys/random.h#L15 > > https://github.com/aosp-mirror/platform_bionic/blob/master/libc/include/sys/random.h#L65 > > And other operating systems implement various alternatives to getrandom(). Do we add FreeBSD's read_random(2), OpenBSD's old sysctl(3) KERN_ARND, MS Windows CryptGenRandom(), and Apple iOS's SecRandom() to NuttX as well? Except iOS and Windows, other POSIX variant free open source OSs(FreeBSD, OpenBSD or Linux) are fine, but not your own definition. > > Solaris 11.3 has getrandom() with this prototype > > `int getrandom(void *buf, size_t buflen, unsigned int flags);` > > while Linux and FreeBSD use ssize_t as return value. Solaris versions semantics are also slightly different, buflen is limited to 1024 and no EINTR is specified and maybe there are more subtle differences as well between Linux, FreeBSD, Solaris and the various libc wrappers? Solaris man page: https://docs.oracle.com/cd/E86824_01/html/E54765/getrandom-2.html > Why we need consider the commerce implementation? > It seems Linux kernel folks have had serious problems with getrandom(), please read > https://lwn.net/Articles/800509/ The discussion show that it's very important to handle the low entropy in correct way(NuttX's has the similar problem) and your simplified prototype hiden the fact and return the unsecure entropy potentially. > So there is more to this than just the function prototype. What should getrandom(..., 0) do for instance on NuttX? Should the default be blocking or non-blocking? It seems this has varied in Linux between kernel versions. I think non-blocking is a better choice and Linux people got that wrong. From the security perspective, the blocking should be the default behaviour, but user could select nonblocking explicitly if he know the returned random may less secure. > > I don't want to have bugs in embedded systems, where system hangs waiting for entropy that will never come, this condition perhaps triggered by unrelated commit that optimizes away some interrrupt activity from some other part of the system. Only reliable defence against that is to provide enough initial entropy so that the random pool never blocks. The exact mechanism for initial entropy has to be left to system designer as it varies depending on HW, factory setup, threat model etc. > You can keep the function prototype but implement the nonblocking behaviour if you want. > > Does INVIOLABLES allow you to define a new API doesn't come from any POSIX like OS? > > getrandom() was added by Jussi Kivilinna in 2017 (commit [dffb8a6](https://github.com/apache/incubator-nuttx/commit/dffb8a67e3e92500651db3eca516dbcfc275311a)) Before that it existed at least from 2015 in our (in-house, but publicly available) NuttX fork. That was few years before INVIOLABLES even existed. Linux added getrandom(2) only about year earlier in 2014. As many current contributors were not active in this project back then, it is not very good idea to impute current criteria back in time for that far. We just try to fix the problem and let's the prototype confirm the public definition. My complain is that you know the prototype is wrong, but refuse we correct the problem. > > > ``` > > 1. Fix the prototype of getrandom to confirm man's defintion > > > > 2. Implement arc4random_buf and remove getrandom > > ``` > > > > > > If you don't like both approach, the best thing is to remove getrandom from the code base. > > getrandom() has been used for years in multiple NuttX products shipped to real users. We don't have a habit of removing existing OS features, especially ones that have been in use for years, just because someone does not like every little detail about them. It isn't implementation detail, it's about the function prototype which is the key contract between caller and callee. > We keep things like netlink sockets or 6LowPAN even while they are not in POSIX > Why we need remove netlink? All netlink follow Linux definition. Removing getrandom just because it doesn't confirm Linux's definition. > Changing the prototype affects existing callers, so if you insist having three argument getrandom() That's because your initial implemntation doesn't follow the practice in the first place. If we don't modify the prototype, all other 3rd party code which call getrandom need to be modified. So the real problem is the initial patch which use the wrong prototype. > then I propose that the same commit renames current function as arc4random_buf() as it really is intended to be a drop-in placement for that OpenBSD derived API. This way existing callers can do a simple rename, without having to figure what flags to pass to getrandom() (I guess none of the flags will be implemented, but that may change in future). Ok, let's rename getrandom to arc4random_buf. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
