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;
}
On 5/19/22 5:20 PM, Jason A. Donenfeld wrote:
> 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;
Yep, that looks good! Do you still want a v2?
--
Jens Axboe