Undefined behaviour in ctr32_encrypt_blocks

[jasonyu1996]

Hi! It looks like this code in ctr32_encrypt_blocks has undefined behaviours.

ring/src/aead/aes/ffi.rs

Lines 174 to 202 in befdc87

	let (input, leftover) = slice::as_chunks(&in_out[src]);
	debug_assert_eq!(leftover.len(), 0);
	let blocks = match NonZeroUsize::new(input.len()) {
	Some(blocks) => blocks,
	None => {
	return;
	}
	};
	let blocks_u32: u32 = blocks.get().try_into().unwrap();
	let input = input.as_ptr();
	let output: *mut [u8; BLOCK_LEN] = in_out.as_mut_ptr().cast();
	// SAFETY:
	// * `input` points to `blocks` blocks.
	// * `output` points to space for `blocks` blocks to be written.
	// * input == output.add(n), where n == src.start, and the caller is
	// responsible for ensuing this sufficient for `f` to work correctly.
	// * The caller is responsible for ensuring `f` can handle any value of
	// `blocks` including zero.
	// * The caller is responsible for ensuring `key` was initialized by the
	// `set_encrypt_key!` invocation required by `f`.
	unsafe {
	f(input, output, blocks, self, ctr);
	}
	ctr.increment_by_less_safe(blocks_u32);

I reproduced the same logic with the FFI replaced with unsafe Rust and MIRI reports undefined behaviours: https://play.rust-lang.org/?version=nightly&mode=release&edition=2021&gist=2b08f1fac9a144fee06c956f14ff598e

The issue is that in_out is borrowed twice, first as &in_out and then as &mut in_out (through in_out.as_mut_ptr()). The latter invalidates anything derived from the former, including input.

[briansmith]

The latter invalidates anything derived from the former, including input.

Could you provide a reference for this rule, other than Miri?

[briansmith]

The latter invalidates anything derived from the former, including input.

I guess the line let output: *mut [u8; BLOCK_LEN] = in_out.as_mut_ptr().cast(); could be moved to be ahead of where the input borrow happens.

However, IDK that it makes sense for input to be considered invalidated with the way the code is now, in part because input is a sub-borrow (terminology?) of in_out. I think this is a difference between "Tree Borrows," which is the actual model used by Rust, and "Stacked Borrows," which is what MIRI uses, right?

[briansmith]

I'll need to check whether MIRIFLAGS=-Zmiri-tree-borrows changes the result.

[briansmith]

PR #2164 addresses this.

[jasonyu1996]

Tree Borrows also considers this a UB. As for the reasoning behind this, we had better consult people working on those aliasing models. They might also be interested in seeing more cases to help refine the models.

Correction: Tree Borrows considers this to be okay as long as input is not used to read from a memory location that has been mutated through output.