Re: [PATCH v2] Add getrandom syscall wrapper and getentropy function

[Hauke Mehrtens <hauke@hauke-m.de>]

On 01/09/2018 07:48 PM, Rich Felker wrote:
> On Sat, Jan 06, 2018 at 11:08:09PM +0100, Hauke Mehrtens wrote:
>> This syscall is available since Linux 3.17 and was also implemented in
>> glibc in version 2.25 using the same interfaces.
>> The getrandom function is a pure syscall wrapper liker glibc does it.
>> getentropy is implemented on top of the getrandom syscall and fills the
>> buffer.
>>
>> Currently no fallback is implemented this could be possible by using
>> AT_RANDOM in the future.
>> ---
>>  include/sys/random.h   | 21 +++++++++++++++++++++
>>  src/linux/getentropy.c | 29 +++++++++++++++++++++++++++++
>>  src/linux/getrandom.c  |  7 +++++++
>>  3 files changed, 57 insertions(+)
>>  create mode 100644 include/sys/random.h
>>  create mode 100644 src/linux/getentropy.c
>>  create mode 100644 src/linux/getrandom.c
>>
>> diff --git a/include/sys/random.h b/include/sys/random.h
>> new file mode 100644
>> index 00000000..5b09e394
>> --- /dev/null
>> +++ b/include/sys/random.h
>> @@ -0,0 +1,21 @@
>> +#ifndef _SYS_RANDOM_H
>> +#define _SYS_RANDOM_H
>> +#ifdef __cplusplus
>> +extern "C" {
>> +#endif
>> +
>> +#define __NEED_size_t
>> +#define __NEED_ssize_t
>> +#include <bits/alltypes.h>
>> +
>> +#define GRND_NONBLOCK	0x0001
>> +#define GRND_RANDOM	0x0002
>> +
>> +ssize_t getrandom(void *buf, size_t buflen, unsigned int flags);
>> +
>> +int getentropy(void *buffer, size_t length);
> 
> Needs to drop namespace pollution in arg lists; just don't use names
> here.

Will do that.

> 
>> +#ifdef __cplusplus
>> +}
>> +#endif
>> +#endif
>> diff --git a/src/linux/getentropy.c b/src/linux/getentropy.c
>> new file mode 100644
>> index 00000000..48ca3d51
>> --- /dev/null
>> +++ b/src/linux/getentropy.c
>> @@ -0,0 +1,29 @@
>> +#include <sys/random.h>
>> +#include <errno.h>
>> +#include "syscall.h"
>> +
>> +int getentropy(void *buffer, size_t length)
>> +{
>> +	int ret;
>> +	char *pos = buffer;
>> +	size_t rem = length;
>> +
>> +	if (length > 256) {
>> +		return __syscall_ret(-EIO);
>> +	}
>> +
>> +	while (rem) {
>> +		ret = __syscall_cp(SYS_getrandom, pos, rem, 0);
> 
> My understanding was that the glibc choice was not to make getentropy
> a cancellation point. If that's correct, is there a reason you want to
> do it differently?

Ok I will change this, glibc made getentropy no cancellation point, but
getrandom is a cancellation point I will do it the same way here.

From glibc documentation:
> The @code{getentropy} function is not a cancellation point.  A call to
> @code{getentropy} can block if the system has just booted and the
> kernel entropy pool has not yet been initialized.  In this case, the
> function will keep blocking even if a signal arrives, and return only
> after the entropy pool has been initialized.

> The @code{getrandom} function is a cancellation point.

Source:
https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=92dcaa3e2f7bf0f7f1c04cd2fb6a317df1a4e225

> 
>> +		if (ret == -EINTR) {
>> +			continue;
> 
> This in particular seems inconsistent with making it cancellable, but
> maybe the idea is to avoid issues where the caller does not check the
> return.
> 
>> +		} else if (ret == -EFAULT || ret == -ENOSYS) {
>> +			return __syscall_ret(ret);
>> +		} else if (ret <= 0) {
>> +			return __syscall_ret(-EIO);
>> +		}
> 
> Is there a reason for collapsing some possible error conditions but
> not others?

The man page said that getentropy only returns, EFAULT, EIO or ENOSYS.
Should I forward all errors?

>> +
>> +		pos += ret;
>> +		rem -= ret;
>> +	}
>> +	return 0;
>> +}
> 
> Overall, my leaning for getentropy would be to implement it as a
> wrapper around getrandom using the public interfaces (e.g.
> pthread_setcancelstate if cancellation is to be blocked, and errno
> rather than -EFOO stuff), with the idea that it should not need to be
> changed if we later implement fallbacks for getrandom and that it be a
> "generic" file that not depend on musl internals.

In glibc it is not a cancellation point, so should I call the syscall
directly with no cancellation point or should I call the getrandom())
function which has a cancellation point?

>> diff --git a/src/linux/getrandom.c b/src/linux/getrandom.c
>> new file mode 100644
>> index 00000000..795b932c
>> --- /dev/null
>> +++ b/src/linux/getrandom.c
>> @@ -0,0 +1,7 @@
>> +#include <sys/random.h>
>> +#include "syscall.h"
>> +
>> +ssize_t getrandom(void *buf, size_t buflen, unsigned int flags)
>> +{
>> +	return syscall_cp(SYS_getrandom, buf, buflen, flags);
>> +}
> 
> This part looks fine, I think.
> 
> Rich
> 

Hauke