Rather than failing entirely if a copy_to_user() fails at some point,
instead we should return a partial read for the amount that succeeded
prior, unless none succeeded at all, in which case we return -EFAULT as
before.
This makes it consistent with other reader interfaces. For example, the
following snippet for /dev/zero outputs "4" followed by "1":
int fd;
void *x = mmap(NULL, 4096, PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
assert(x != MAP_FAILED);
fd = open("/dev/zero", O_RDONLY);
assert(fd >= 0);
printf("%zd\n", read(fd, x, 4));
printf("%zd\n", read(fd, x + 4095, 4));
close(fd);
This brings that same standard behavior to the various RNG reader
interfaces.
While we're at it, we can streamline the loop logic a little bit.
Suggested-by: Linus Torvalds <[email protected]>
Cc: Jann Horn <[email protected]>
Signed-off-by: Jason A. Donenfeld <[email protected]>
---
drivers/char/random.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index e15063d61460..dd31fddecd17 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -523,8 +523,7 @@ EXPORT_SYMBOL(get_random_bytes);
static ssize_t get_random_bytes_user(void __user *buf, size_t nbytes)
{
- ssize_t ret = 0;
- size_t len;
+ size_t len, ret = 0;
u32 chacha_state[CHACHA_STATE_WORDS];
u8 output[CHACHA_BLOCK_SIZE];
@@ -543,37 +542,37 @@ static ssize_t get_random_bytes_user(void __user *buf, size_t nbytes)
* the user directly.
*/
if (nbytes <= CHACHA_KEY_SIZE) {
- ret = copy_to_user(buf, &chacha_state[4], nbytes) ? -EFAULT : nbytes;
+ ret = copy_to_user(buf, &chacha_state[4], nbytes) ? 0 : nbytes;
goto out_zero_chacha;
}
- do {
+ for (;;) {
chacha20_block(chacha_state, output);
if (unlikely(chacha_state[12] == 0))
++chacha_state[13];
len = min_t(size_t, nbytes, CHACHA_BLOCK_SIZE);
- if (copy_to_user(buf, output, len)) {
- ret = -EFAULT;
+ if (copy_to_user(buf, output, len))
break;
- }
- nbytes -= len;
buf += len;
ret += len;
+ nbytes -= len;
+ if (!nbytes)
+ break;
BUILD_BUG_ON(PAGE_SIZE % CHACHA_BLOCK_SIZE != 0);
- if (!(ret % PAGE_SIZE) && nbytes) {
+ if (!(ret % PAGE_SIZE)) {
if (signal_pending(current))
break;
cond_resched();
}
- } while (nbytes);
+ }
memzero_explicit(output, sizeof(output));
out_zero_chacha:
memzero_explicit(chacha_state, sizeof(chacha_state));
- return ret;
+ return ret ? ret : -EFAULT;
}
/*
--
2.35.1
Rather than failing entirely if a copy_to_user() fails at some point,
instead we should return a partial read for the amount that succeeded
prior, unless none succeeded at all, in which case we return -EFAULT as
before.
This makes it consistent with other reader interfaces. For example, the
following snippet for /dev/zero outputs "4" followed by "1":
int fd;
void *x = mmap(NULL, 4096, PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
assert(x != MAP_FAILED);
fd = open("/dev/zero", O_RDONLY);
assert(fd >= 0);
printf("%zd\n", read(fd, x, 4));
printf("%zd\n", read(fd, x + 4095, 4));
close(fd);
This brings that same standard behavior to the various RNG reader
interfaces.
While we're at it, we can streamline the loop logic a little bit.
Suggested-by: Linus Torvalds <[email protected]>
Cc: Jann Horn <[email protected]>
Signed-off-by: Jason A. Donenfeld <[email protected]>
---
Changes v1->v2:
- Do partial copies within individual blocks, not just per-block, also
following how /dev/zero and ordinary filesystem files work.
drivers/char/random.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index e15063d61460..df43c5060f00 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -523,8 +523,7 @@ EXPORT_SYMBOL(get_random_bytes);
static ssize_t get_random_bytes_user(void __user *buf, size_t nbytes)
{
- ssize_t ret = 0;
- size_t len;
+ size_t len, left, ret = 0;
u32 chacha_state[CHACHA_STATE_WORDS];
u8 output[CHACHA_BLOCK_SIZE];
@@ -543,37 +542,40 @@ static ssize_t get_random_bytes_user(void __user *buf, size_t nbytes)
* the user directly.
*/
if (nbytes <= CHACHA_KEY_SIZE) {
- ret = copy_to_user(buf, &chacha_state[4], nbytes) ? -EFAULT : nbytes;
+ ret = nbytes - copy_to_user(buf, &chacha_state[4], nbytes);
goto out_zero_chacha;
}
- do {
+ for (;;) {
chacha20_block(chacha_state, output);
if (unlikely(chacha_state[12] == 0))
++chacha_state[13];
len = min_t(size_t, nbytes, CHACHA_BLOCK_SIZE);
- if (copy_to_user(buf, output, len)) {
- ret = -EFAULT;
+ left = copy_to_user(buf, output, len);
+ if (left) {
+ ret += len - left;
break;
}
- nbytes -= len;
buf += len;
ret += len;
+ nbytes -= len;
+ if (!nbytes)
+ break;
BUILD_BUG_ON(PAGE_SIZE % CHACHA_BLOCK_SIZE != 0);
- if (!(ret % PAGE_SIZE) && nbytes) {
+ if (ret % PAGE_SIZE == 0) {
if (signal_pending(current))
break;
cond_resched();
}
- } while (nbytes);
+ }
memzero_explicit(output, sizeof(output));
out_zero_chacha:
memzero_explicit(chacha_state, sizeof(chacha_state));
- return ret;
+ return ret ? ret : -EFAULT;
}
/*
--
2.35.1
From: Jason A. Donenfeld
> Sent: 08 April 2022 00:36
>
> Rather than failing entirely if a copy_to_user() fails at some point,
> instead we should return a partial read for the amount that succeeded
> prior, unless none succeeded at all, in which case we return -EFAULT as
> before.
I think you now return -EFAULT for a zero length read.
David
>
> This makes it consistent with other reader interfaces. For example, the
> following snippet for /dev/zero outputs "4" followed by "1":
>
> int fd;
> void *x = mmap(NULL, 4096, PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
> assert(x != MAP_FAILED);
> fd = open("/dev/zero", O_RDONLY);
> assert(fd >= 0);
> printf("%zd\n", read(fd, x, 4));
> printf("%zd\n", read(fd, x + 4095, 4));
> close(fd);
>
> This brings that same standard behavior to the various RNG reader
> interfaces.
>
> While we're at it, we can streamline the loop logic a little bit.
>
> Suggested-by: Linus Torvalds <[email protected]>
> Cc: Jann Horn <[email protected]>
> Signed-off-by: Jason A. Donenfeld <[email protected]>
> ---
> Changes v1->v2:
> - Do partial copies within individual blocks, not just per-block, also
> following how /dev/zero and ordinary filesystem files work.
>
> drivers/char/random.c | 22 ++++++++++++----------
> 1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index e15063d61460..df43c5060f00 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -523,8 +523,7 @@ EXPORT_SYMBOL(get_random_bytes);
>
> static ssize_t get_random_bytes_user(void __user *buf, size_t nbytes)
> {
> - ssize_t ret = 0;
> - size_t len;
> + size_t len, left, ret = 0;
> u32 chacha_state[CHACHA_STATE_WORDS];
> u8 output[CHACHA_BLOCK_SIZE];
>
> @@ -543,37 +542,40 @@ static ssize_t get_random_bytes_user(void __user *buf, size_t nbytes)
> * the user directly.
> */
> if (nbytes <= CHACHA_KEY_SIZE) {
> - ret = copy_to_user(buf, &chacha_state[4], nbytes) ? -EFAULT : nbytes;
> + ret = nbytes - copy_to_user(buf, &chacha_state[4], nbytes);
> goto out_zero_chacha;
> }
>
> - do {
> + for (;;) {
> chacha20_block(chacha_state, output);
> if (unlikely(chacha_state[12] == 0))
> ++chacha_state[13];
>
> len = min_t(size_t, nbytes, CHACHA_BLOCK_SIZE);
> - if (copy_to_user(buf, output, len)) {
> - ret = -EFAULT;
> + left = copy_to_user(buf, output, len);
> + if (left) {
> + ret += len - left;
> break;
> }
>
> - nbytes -= len;
> buf += len;
> ret += len;
> + nbytes -= len;
> + if (!nbytes)
> + break;
>
> BUILD_BUG_ON(PAGE_SIZE % CHACHA_BLOCK_SIZE != 0);
> - if (!(ret % PAGE_SIZE) && nbytes) {
> + if (ret % PAGE_SIZE == 0) {
> if (signal_pending(current))
> break;
> cond_resched();
> }
> - } while (nbytes);
> + }
>
> memzero_explicit(output, sizeof(output));
> out_zero_chacha:
> memzero_explicit(chacha_state, sizeof(chacha_state));
> - return ret;
> + return ret ? ret : -EFAULT;
> }
>
> /*
> --
> 2.35.1
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Hi David,
On 4/8/22, David Laight <[email protected]> wrote:
> From: Jason A. Donenfeld
>> Sent: 08 April 2022 00:36
>>
>> Rather than failing entirely if a copy_to_user() fails at some point,
>> instead we should return a partial read for the amount that succeeded
>> prior, unless none succeeded at all, in which case we return -EFAULT as
>> before.
>
> I think you now return -EFAULT for a zero length read.
The diff context doesn't show it, but the first line of the function
is `if (!nbytes) return 0;`, before various other bits of work are
done.
Jason