1. Patchew
  2. linux
  3. View series

[PATCH v2] random: allow partial reads if later user copies fail

Jason A. Donenfeld posted 1 patch 4 years, 1 month ago 
drivers/char/random.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
[PATCH v2] random: allow partial reads if later user copies fail
Posted by Jason A. Donenfeld 4 years, 1 month ago 
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 <torvalds@linux-foundation.org>
Cc: Jann Horn <jannh@google.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
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
RE: [PATCH v2] random: allow partial reads if later user copies fail
Posted by David Laight 4 years, 1 month ago 
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 <torvalds@linux-foundation.org>
> Cc: Jann Horn <jannh@google.com>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
> 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)
Re: [PATCH v2] random: allow partial reads if later user copies fail
Posted by Jason A. Donenfeld 4 years, 1 month ago 
Hi David,

On 4/8/22, David Laight <David.Laight@aculab.com> 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

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.