Skip to content

Commit f2a5213

Browse files
authored
cmov: fix provided Cmov::cmovz impl (#1351)
We didn't notice it was broken because all of the impls for all of the core types in the crate explicitly write both methods. However, #1350 just impl'd `Cmov` for `[u8; N]` and didn't have an impl of `cmovz` because the `cmovnz` impl is non-trivial and ideally we could just rely on the provided method, but the definition on the trait was just doing `!condition`, which will only flip a non-zero value into a zero value if provided `$int::MAX`. Though I was happy to eliminate macros in #1339, this unfortunately brings them back, adapting the previous `bitnz!` macro used in the `portable` backend into a new internal `masknz!` macro placed in a new `macros` module accessible to the whole crate. Though it's annoying to use macros again, #1339 duplicated the definition twice, and for such a touchy piece of code with the potential to introduce non-constant-time behavior (see CVE-2026-23519) it's nice to have it defined in a single place. The default implementation of `Cmov::cmovz` has been changed to use the new `masknz!` macro to output `u8::MAX` for non-zero values, which we can invert with `!` to get `0`, and `0` will invert to `u8::MAX`, which is the behavior we wanted. The `masknz!` macro has also been used to write `masknz32!` and `masknz64!` in the portable backend, which were previously duplicated.
1 parent 4bb07d1 commit f2a5213

File tree

4 files changed

+78
-7
lines changed

4 files changed

+78
-7
lines changed

cmov/src/lib.rs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,9 @@
2727
unused_qualifications
2828
)]
2929

30+
#[macro_use]
31+
mod macros;
32+
3033
#[cfg(not(miri))]
3134
#[cfg(target_arch = "aarch64")]
3235
mod aarch64;
@@ -57,7 +60,8 @@ pub trait Cmov {
5760
/// equal to zero, and if so, conditionally moves `value` to `self`
5861
/// when `condition` is equal to zero.
5962
fn cmovz(&mut self, value: &Self, condition: Condition) {
60-
self.cmovnz(value, !condition)
63+
let nz = masknz!(condition: Condition);
64+
self.cmovnz(value, !nz)
6165
}
6266
}
6367

cmov/src/macros.rs

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
//! Macro definitions.
2+
3+
/// Generates a mask the width of the input type if the input value is non-zero.
4+
///
5+
/// Uses `core::hint::black_box` to coerce our desired codegen based on real-world observations
6+
/// of the assembly generated by Rust/LLVM.
7+
///
8+
/// See also:
9+
/// - CVE-2026-23519
10+
/// - RustCrypto/utils#1332
11+
macro_rules! masknz {
12+
($value:tt : $int:ident) => {{
13+
let mut value: $int = $value;
14+
value |= value.wrapping_neg(); // has MSB `1` if non-zero, `0` if zero
15+
16+
// use `black_box` to obscure we're computing a 1-bit value
17+
core::hint::black_box(
18+
value >> ($int::BITS - 1), // Extract MSB
19+
)
20+
.wrapping_neg() // Generate $int::MAX mask if `black_box` outputs `1`
21+
}};
22+
}
23+
24+
#[cfg(test)]
25+
mod tests {
26+
// Spot check up to a given limit
27+
const TEST_LIMIT: u8 = 128;
28+
29+
macro_rules! masknz_test {
30+
( $($name:ident : $int:ident),+ ) => {
31+
$(
32+
#[test]
33+
fn $name() {
34+
assert_eq!(masknz!(0: $int), 0);
35+
36+
// Test lower values
37+
for i in 1..=$int::from(TEST_LIMIT) {
38+
assert_eq!(masknz!(i: $int), $int::MAX);
39+
}
40+
41+
// Test upper values
42+
for i in ($int::MAX - $int::from(TEST_LIMIT))..=$int::MAX {
43+
assert_eq!(masknz!(i: $int), $int::MAX);
44+
}
45+
}
46+
)+
47+
}
48+
}
49+
50+
// Ensure the macro works with any types we might use it with (we only use u8, u32, and u64)
51+
masknz_test!(
52+
masknz_u8: u8,
53+
masknz_u16: u16,
54+
masknz_u32: u32,
55+
masknz_u64: u64,
56+
masknz_u128: u128
57+
);
58+
}

cmov/src/portable.rs

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -118,17 +118,13 @@ fn maskne64(x: u64, y: u64) -> u64 {
118118
/// Return a [`u32::MAX`] mask if `condition` is non-zero, otherwise return zero for a zero input.
119119
#[cfg(not(target_arch = "arm"))]
120120
fn masknz32(condition: u32) -> u32 {
121-
let x = condition | condition.wrapping_neg(); // MSB of `x` now `1` if non-zero
122-
let nz = core::hint::black_box(x >> (u32::BITS - 1)); // Extract MSB
123-
nz.wrapping_neg()
121+
masknz!(condition: u32)
124122
}
125123

126124
/// Return a [`u64::MAX`] mask if `condition` is non-zero, otherwise return zero for a zero input.
127125
#[cfg(not(target_arch = "arm"))]
128126
fn masknz64(condition: u64) -> u64 {
129-
let x = condition | condition.wrapping_neg(); // MSB of `x` now `1` if non-zero
130-
let nz = core::hint::black_box(x >> (u64::BITS - 1)); // Extract MSB
131-
nz.wrapping_neg()
127+
masknz!(condition: u64)
132128
}
133129

134130
/// Optimized mask generation for ARM32 targets.

cmov/tests/core_impls.rs

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,19 @@ mod arrays {
166166
assert_eq!(x, EXAMPLE_B);
167167
}
168168
}
169+
170+
#[test]
171+
fn u8_cmovz_works() {
172+
let mut x = EXAMPLE_A;
173+
x.cmovz(&EXAMPLE_B, 0);
174+
assert_eq!(x, EXAMPLE_B);
175+
176+
for cond in 1..u8::MAX {
177+
let mut x = EXAMPLE_A;
178+
x.cmovz(&EXAMPLE_B, cond);
179+
assert_eq!(x, EXAMPLE_A);
180+
}
181+
}
169182
}
170183

171184
mod slices {

0 commit comments

Comments
 (0)