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

From: David Laight
Date: Fri Apr 08 2022 - 04:11:46 EST


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@xxxxxxxxxxxxxxxxxxxx>
> Cc: Jann Horn <jannh@xxxxxxxxxx>
> Signed-off-by: Jason A. Donenfeld <Jason@xxxxxxxxx>
> ---
> 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)