linux-kernel - RE: [PATCH v2] random: allow partial reads if later user copies fail

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 8 Apr 2022 08:11:35 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     "'Jason A. Donenfeld'" <Jason@...c4.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "torvalds@...ux-foundation.org" <torvalds@...ux-foundation.org>
CC:     "tytso@....edu" <tytso@....edu>,
        "sultan@...neltoast.com" <sultan@...neltoast.com>,
        Jann Horn <jannh@...gle.com>
Subject: RE: [PATCH v2] random: allow partial reads if later user copies fail

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@...ux-foundation.org>
> Cc: Jann Horn <jannh@...gle.com>
> Signed-off-by: Jason A. Donenfeld <Jason@...c4.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)