Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   David Laight <David.Laight@ACULAB.COM>
To:     "'Jason A. Donenfeld'" <Jason@zx2c4.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "torvalds@linux-foundation.org" <torvalds@linux-foundation.org>
CC:     "tytso@mit.edu" <tytso@mit.edu>,
        "sultan@kerneltoast.com" <sultan@kerneltoast.com>,
        Jann Horn <jannh@google.com>
Subject: RE: [PATCH v2] random: allow partial reads if later user copies fail
Thread-Topic: [PATCH v2] random: allow partial reads if later user copies fail
Thread-Index: AQHYStia62LV1RpflEWlDISfQ8tDQqzlqpzQ
Date:   Fri, 8 Apr 2022 08:11:35 +0000
Message-ID: <8d652dac67754a308b270c453b3032d2@AcuMS.aculab.com>
References: <20220407193433.523299-1-Jason@zx2c4.com>
 <20220407233558.3369-1-Jason@zx2c4.com>
In-Reply-To: <20220407233558.3369-1-Jason@zx2c4.com>
Accept-Language: en-GB, en-US
MIME-Version: 1.0
Content-Language: en-US
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8BIT
Precedence: bulk

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)