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;
Date:   Fri, 20 May 2022 01:20:23 +0200
From:   "Jason A. Donenfeld" <Jason@zx2c4.com>
To:     Jens Axboe <axboe@kernel.dk>
Cc:     tytso@mit.edu, hch@lst.de, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] random: convert to using fops->read_iter()
Message-ID: <YobQt1Fbae0KtEFw@zx2c4.com>
References: <20220519193133.194138-1-axboe@kernel.dk>
 <20220519193133.194138-2-axboe@kernel.dk>
 <YobOxLKnRDyYChXS@zx2c4.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <YobOxLKnRDyYChXS@zx2c4.com>
Precedence: bulk

On Fri, May 20, 2022 at 01:12:04AM +0200, Jason A. Donenfeld wrote:
> Hi Jens,
> 
> On Thu, May 19, 2022 at 01:31:32PM -0600, Jens Axboe wrote:
> >  	for (;;) {
> >  		chacha20_block(chacha_state, output);
> >  		if (unlikely(chacha_state[12] == 0))
> >  			++chacha_state[13];
> >  
> >  		block_len = min_t(size_t, len, CHACHA_BLOCK_SIZE);
> > -		left = copy_to_user(ubuf, output, block_len);
> > -		if (left) {
> > -			ret += block_len - left;
> > +		block_len = copy_to_iter(output, block_len, to);
> > +		if (!block_len)
> >  			break;
> > -		}
> >  
> > -		ubuf += block_len;
> >  		ret += block_len;
> >  		len -= block_len;
> > -		if (!len)
> > -			break;
> >  
> >  		BUILD_BUG_ON(PAGE_SIZE % CHACHA_BLOCK_SIZE != 0);
> >  		if (ret % PAGE_SIZE == 0) {
> >  			if (signal_pending(current))
> >  				break;
> >  			cond_resched();
> >  		}
> >  	}
> 
> This isn't quite the same, is it? Before, it would immediately break out
> of the loop on any short copy. Now, it will only break out on a zero
> copy, which means it's possible that ret % PAGE_SIZE == 0, and there'll
> be an unnecessary cond_resched() before copy_to_iter() runs again and
> then breaks.

Maybe something like the below would do the trick?


static ssize_t get_random_bytes_user(struct iov_iter *to)
{
	size_t block_len, copied, ret = 0, len = iov_iter_count(to);
	u32 chacha_state[CHACHA_STATE_WORDS];
	u8 output[CHACHA_BLOCK_SIZE];

	if (!len)
		return 0;

	/*
	 * Immediately overwrite the ChaCha key at index 4 with random
	 * bytes, in case userspace causes copy_to_user() below to sleep
	 * forever, so that we still retain forward secrecy in that case.
	 */
	crng_make_state(chacha_state, (u8 *)&chacha_state[4], CHACHA_KEY_SIZE);
	/*
	 * However, if we're doing a read of len <= 32, we don't need to
	 * use chacha_state after, so we can simply return those bytes to
	 * the user directly.
	 */
	if (len <= CHACHA_KEY_SIZE) {
		ret = copy_to_iter(&chacha_state[4], len, to);
		goto out_zero_chacha;
	}

	for (;;) {
		chacha20_block(chacha_state, output);
		if (unlikely(chacha_state[12] == 0))
			++chacha_state[13];

		block_len = min_t(size_t, len, CHACHA_BLOCK_SIZE);
		copied = copy_to_iter(output, block_len, to);
		ret += copied;
		if (block_len != copied)
			break;
		len -= copied;

		BUILD_BUG_ON(PAGE_SIZE % CHACHA_BLOCK_SIZE != 0);
		if (ret % PAGE_SIZE == 0) {
			if (signal_pending(current))
				break;
			cond_resched();
		}
	}

	memzero_explicit(output, sizeof(output));
out_zero_chacha:
	memzero_explicit(chacha_state, sizeof(chacha_state));
	return ret ? ret : -EFAULT;
}