Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Use zerocopy byteorder module to replace endianness stuff #1693

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ name = "ring"
[dependencies]
getrandom = { version = "0.2.8" }
untrusted = { version = "0.9" }
zerocopy = { version = "0.7.8", features = ["byteorder"] }

[target.'cfg(any(target_arch = "x86",target_arch = "x86_64", all(any(target_arch = "aarch64", target_arch = "arm"), any(target_os = "android", target_os = "fuchsia", target_os = "linux", target_os = "windows"))))'.dependencies]
spin = { version = "0.9.2", default-features = false, features = ["once"] }
Expand Down
11 changes: 5 additions & 6 deletions src/aead/aes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,11 @@ use super::{
};
use crate::{
bits::BitLength,
c, cpu,
endian::{ArrayEncoding, BigEndian},
error,
c, cpu, error,
polyfill::{self, ChunksFixed},
};
use core::ops::RangeFrom;
use zerocopy::byteorder::big_endian::U32 as BEU32;

#[derive(Clone)]
pub(super) struct Key {
Expand Down Expand Up @@ -323,7 +322,7 @@ pub enum Variant {

/// Nonce || Counter, all big-endian.
#[repr(transparent)]
pub(super) struct Counter([BigEndian<u32>; 4]);
pub(super) struct Counter([BEU32; 4]);

impl Counter {
pub fn one(nonce: Nonce) -> Self {
Expand All @@ -346,7 +345,7 @@ impl Counter {
/// The IV for a single block encryption.
///
/// Intentionally not `Clone` to ensure each is used only once.
pub struct Iv([BigEndian<u32>; 4]);
pub struct Iv([BEU32; 4]);

impl From<Counter> for Iv {
fn from(counter: Counter) -> Self {
Expand All @@ -356,7 +355,7 @@ impl From<Counter> for Iv {

impl Iv {
pub(super) fn as_bytes_less_safe(&self) -> &[u8; 16] {
self.0.as_byte_array()
zerocopy::transmute_ref!(&self.0)
briansmith marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once we import in the benchmarks for ring::{digest,hmac,pbkdf2} then we can evaluate the performance of PR #1731. I expect that even if PR #1731 as written has performance issues, we will be able to tweak it so it doesn't. This will almost definitely eliminate this code completely.

}
}

Expand Down
10 changes: 5 additions & 5 deletions src/aead/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,19 +12,19 @@
// OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
// CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.

use crate::endian::*;
use core::ops::{BitXor, BitXorAssign};
use zerocopy::byteorder::big_endian::U64 as BEU64;

#[repr(transparent)]
#[derive(Copy, Clone)]
pub struct Block([BigEndian<u64>; 2]);
pub struct Block([BEU64; 2]);

pub const BLOCK_LEN: usize = 16;

impl Block {
#[inline]
pub fn zero() -> Self {
Self([Encoding::ZERO; 2])
Self([BEU64::ZERO; 2])
}

#[inline]
Expand Down Expand Up @@ -79,13 +79,13 @@ impl BitXor for Block {
impl From<&'_ [u8; BLOCK_LEN]> for Block {
#[inline]
fn from(bytes: &[u8; BLOCK_LEN]) -> Self {
Self(FromByteArray::from_byte_array(bytes))
Self(zerocopy::transmute!(*bytes))
}
}

impl AsRef<[u8; BLOCK_LEN]> for Block {
#[inline]
fn as_ref(&self) -> &[u8; BLOCK_LEN] {
self.0.as_byte_array()
zerocopy::transmute_ref!(&self.0)
}
}
9 changes: 5 additions & 4 deletions src/aead/chacha20_poly1305.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,9 @@
chacha::{self, Counter, Iv},
poly1305, Aad, Nonce, Tag,
};
use crate::{aead, cpu, endian::*, error, polyfill};
use crate::{aead, cpu, error, polyfill};
use core::ops::RangeFrom;
use zerocopy::{byteorder::little_endian::U64 as LEU64, AsBytes as _};

/// ChaCha20-Poly1305 as described in [RFC 8439].
///
Expand Down Expand Up @@ -209,10 +210,10 @@
fn finish(mut auth: poly1305::Context, aad_len: usize, in_out_len: usize) -> Tag {
auth.update(
[
LittleEndian::from(polyfill::u64_from_usize(aad_len)),
LittleEndian::from(polyfill::u64_from_usize(in_out_len)),
LEU64::from(polyfill::u64_from_usize(aad_len)),
LEU64::from(polyfill::u64_from_usize(in_out_len)),

Check warning on line 214 in src/aead/chacha20_poly1305.rs

View check run for this annotation

Codecov / codecov/patch

src/aead/chacha20_poly1305.rs#L213-L214

Added lines #L213 - L214 were not covered by tests
]
.as_byte_array(),
.as_bytes(),

Check warning on line 216 in src/aead/chacha20_poly1305.rs

View check run for this annotation

Codecov / codecov/patch

src/aead/chacha20_poly1305.rs#L216

Added line #L216 was not covered by tests
);
auth.finish()
}
Expand Down
11 changes: 4 additions & 7 deletions src/aead/chacha20_poly1305_openssh.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ use super::{
polyfill::ChunksFixed,
Nonce, Tag,
};
use crate::{constant_time, endian::*, error};
use crate::{constant_time, error};
use zerocopy::byteorder::big_endian::U32 as BEU32;

/// A key for sealing packets.
pub struct SealingKey {
Expand Down Expand Up @@ -161,12 +162,8 @@ impl Key {
}

fn make_counter(sequence_number: u32) -> Counter {
let nonce = [
BigEndian::ZERO,
BigEndian::ZERO,
BigEndian::from(sequence_number),
];
Counter::zero(Nonce::assume_unique_for_key(*(nonce.as_byte_array())))
let nonce = [BEU32::ZERO, BEU32::ZERO, BEU32::from(sequence_number)];
Counter::zero(Nonce::assume_unique_for_key(zerocopy::transmute!(nonce)))
}

/// The length of key.
Expand Down
53 changes: 39 additions & 14 deletions src/digest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,12 @@
// The goal for this implementation is to drive the overhead as close to zero
// as possible.

use crate::{
c, cpu, debug,
endian::{ArrayEncoding, BigEndian},
polyfill,
};
use crate::{c, cpu, debug, polyfill};
use core::num::Wrapping;
use zerocopy::{
byteorder::big_endian::{U32 as BEU32, U64 as BEU64},
AsBytes, FromBytes,
};

mod sha1;
mod sha2;
Expand Down Expand Up @@ -114,7 +114,8 @@ impl BlockContext {

Digest {
algorithm: self.algorithm,
value: (self.algorithm.format_output)(self.state),
// SAFETY: TODO
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR #1731 will (I hope) address this as well, by making the code 100% safe as long as we are happy with the safety guarantees for the code that writes to the self.state union.

Which functions within digest need to be marked unsafe and which can be safe with unsafe blocks is something that could use discussion as it seems we may disagree, which means I might be misunderstanding something.

value: unsafe { (self.algorithm.format_output)(self.state) },
}
}
}
Expand Down Expand Up @@ -249,7 +250,7 @@ impl AsRef<[u8]> for Digest {
#[inline(always)]
fn as_ref(&self) -> &[u8] {
let as64 = unsafe { &self.value.as64 };
&as64.as_byte_array()[..self.algorithm.output_len]
&as64.as_bytes()[..self.algorithm.output_len]
}
}

Expand All @@ -270,7 +271,7 @@ pub struct Algorithm {
len_len: usize,

block_data_order: unsafe extern "C" fn(state: &mut State, data: *const u8, num: c::size_t),
format_output: fn(input: State) -> Output,
format_output: unsafe fn(input: State) -> Output,

initial_state: State,

Expand Down Expand Up @@ -458,8 +459,8 @@ union State {
#[derive(Clone, Copy)]
#[repr(C)]
union Output {
as64: [BigEndian<u64>; 512 / 8 / core::mem::size_of::<BigEndian<u64>>()],
as32: [BigEndian<u32>; 256 / 8 / core::mem::size_of::<BigEndian<u32>>()],
as64: [BEU64; 512 / 8 / core::mem::size_of::<BEU64>()],
as32: [BEU32; 256 / 8 / core::mem::size_of::<BEU32>()],
}

/// The maximum block length ([`Algorithm::block_len()`]) of all the algorithms
Expand All @@ -474,17 +475,41 @@ pub const MAX_OUTPUT_LEN: usize = 512 / 8;
/// algorithms in this module.
pub const MAX_CHAINING_LEN: usize = MAX_OUTPUT_LEN;

fn sha256_format_output(input: State) -> Output {
fn sha256_format_output(input: State) -> Output
where
[BEU32; 256 / 8 / core::mem::size_of::<BEU32>()]: FromBytes,
[BEU64; 512 / 8 / core::mem::size_of::<BEU64>()]: AsBytes,
{
// SAFETY: There are two cases:
// - The union is initialized as `as32`, in which case this is trivially
// sound.
// - The union is initialized as `as64`. In this case, the `as64` variant is
// longer than the `as32` variant, so all bytes of `as32` are initialized
// as they are in the prefix of `as64`. Since `as64`'s type is `AsBytes`
// (see the where bound on this function), all of its bytes are
// initialized (ie, none are padding). Since `as32`'s type is `FromBytes`,
// any initialized sequence of bytes constitutes a valid instance of the
// type, so this is sound.
let input = unsafe { &input.as32 };
Output {
as32: input.map(BigEndian::from),
as32: input.map(|n| BEU32::new(n.0)),
}
}

fn sha512_format_output(input: State) -> Output {
/// # Safety
///
/// The caller must ensure that all bytes of `State` have been initialized.
unsafe fn sha512_format_output(input: State) -> Output
where
[BEU64; 512 / 8 / core::mem::size_of::<BEU64>()]: FromBytes,
{
// SAFETY: Caller has promised that all bytes are initialized. Since
// `input.as64` is `FromBytes`, we know that this is sufficient to guarantee
// that the input has been initialized to a valid instance of the type of
// `input.as64`.
let input = unsafe { &input.as64 };
Output {
as64: input.map(BigEndian::from),
as64: input.map(|n| BEU64::new(n.0)),
}
}

Expand Down
139 changes: 0 additions & 139 deletions src/endian.rs

This file was deleted.

1 change: 0 additions & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,6 @@ pub mod io;
mod cpu;
pub mod digest;
mod ec;
mod endian;
pub mod error;
pub mod hkdf;
pub mod hmac;
Expand Down
Loading