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

fix: correct checksum for misaligned slices #148

Merged
merged 3 commits into from
May 21, 2024
Merged
Show file tree
Hide file tree
Changes from 2 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
2 changes: 2 additions & 0 deletions .github/workflows/rust.yml
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,8 @@ jobs:
- run: cargo install [email protected]
- name: Install SCION and run local topology
uses: "./.github/actions/setup-scion"
with:
scion-ref: v0.11.0
id: scion

- name: Run tests (including integration tests) and record coverage
Expand Down
45 changes: 29 additions & 16 deletions crates/scion-proto/src/packet/checksum.rs
mlegner marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -118,35 +118,48 @@ impl ChecksumDigest {
///
/// If the slice is not a multiple of 2-bytes, then it is zero-padded
/// before being added to the checksum.
pub fn add_slice(&mut self, data: &[u8]) -> &mut Self {
pub fn add_slice(&mut self, mut data: &[u8]) -> &mut Self {
if data.is_empty() {
return self;
}
let mut initial_sum = 0;

// Before converting to a `&[u16]` we need to make sure the slice is aligned.
let is_aligned = data.as_ptr().align_offset(2) == 0;
if !is_aligned {
// We want to zero-prepend the value, i.e., for slice where we pair the elements, we
// have [0, A], [B, C], ... Storing [0, X] on a little endian architecture gets written
// as [X, 0] to memory, so we need to swap it with `to_be()`.
initial_sum = (data[0] as u16).to_be() as u32;
data = &data[1..];
};
let ptr: *const u8 = data.as_ptr();

// Converting to a &[u16] requires an even number of elements in the slice
let (data, initial_sum) = if data.len() % 2 == 0 {
(data, 0u32)
} else {
(
&data[..data.len() - 1],
// We want to zero pad the value, i.e., for slice where we pair the elements,
// we have [A, B], [C, D], ... [X, 0]. Since all the values are currently in
// memory in the order [A, B] storing [0, X] on a little endian architecture
// gets written as [X, 0] to memory. On big-endian this would get written as
// [0, X] so we swap it only on that big-endian architectures with to_le()
(data[data.len() - 1] as u16).to_le() as u32,
)
// Converting to a `&[u16]` requires an even number of elements in the slice.
if data.len() % 2 != 0 {
// We want to zero pad the value, i.e., for slice where we pair the elements,
// we have [A, B], [C, D], ... [X, 0]. Since all the values are currently in
// memory in the order [A, B] storing [0, X] on a little endian architecture
// gets written as [X, 0] to memory. On big-endian this would get written as
// [0, X] so we swap it only on that big-endian architectures with to_le().
initial_sum += (data[data.len() - 1] as u16).to_le() as u32;
data = &data[..data.len() - 1];
};

let ptr: *const u8 = data.as_ptr();
let data_u16 = unsafe { slice::from_raw_parts(ptr as *const u16, data.len() / 2) };

let sum_with_overflow = data_u16
.iter()
.fold(initial_sum, |sum, value| sum + (*value as u32));

// Already incorporate the overflow, as it simplifies the endian conversion below
let sum = Self::fold_checksum(sum_with_overflow) as u16;
let mut sum = Self::fold_checksum(sum_with_overflow) as u16;

// If the original slice was not aligned, we need to swap the bytes to get the correct
// checksum.
if !is_aligned {
sum = sum.swap_bytes();
}

// The above sum is actually in big-endian but stored in big/little endian depending
// on the platform. If the platform is little endian, this call will swap the byte-order
Expand Down
4 changes: 1 addition & 3 deletions crates/scion-proto/src/path/standard.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,9 +126,7 @@ where
///
/// There are always at most 3 segments.
pub fn segment(&self, segment_index: usize) -> Option<Segment> {
let Some(info_field) = self.info_field(segment_index) else {
return None;
};
let info_field = self.info_field(segment_index)?;

// Get the index of the first hop field in the segment.
// This is equivalent to the index after all preceding hop fields.
Expand Down
4 changes: 1 addition & 3 deletions crates/scion/src/pan/path_strategy/refresher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -341,9 +341,7 @@ impl<'a> Iterator for PathsTo<'a> {
type Item = &'a Path;

fn next(&mut self) -> Option<Self::Item> {
let Some(paths) = self.paths else {
return None;
};
let paths = self.paths?;

#[allow(clippy::while_let_on_iterator)]
while let Some(info) = self.inner.next() {
Expand Down
Loading