MsgHdrMut::with_addr derives msg_name from a shared reborrow, so recvmsg writes through SharedReadOnly provenance #673

MsgHdrMut was added in #447 specifically so that the address the kernel writes into is passed as
&mut SockAddr. The API change landed, but the implementation still routes that &mut through a
function that takes &SockAddr, so the pointer stored in msg_name ends up with shared
(read-only) provenance. recvmsg then writes through it.

In #447 you wrote:

I think this is unsound when using the non-mutable version in combination with recvmsg, e.g.
recvmsg(fd, &MsgHdr::new().with_addr(&addr)) will write to addr even though we only supplied
a read-only reference, which is UB.

That is exactly the problem; it just survived inside MsgHdrMut as well.

The code (0.6.5 and master)

// src/lib.rs:699-703 (0.6.5: 692-696)
#[allow(clippy::needless_pass_by_ref_mut)]
pub fn with_addr(mut self, addr: &'addr mut SockAddr) -> Self {
    sys::set_msghdr_name(&mut self.inner, addr);   // &mut SockAddr -> &SockAddr
    self
}

// src/sys/unix.rs:828-831 (0.6.5: 773-776); src/sys/windows.rs is the same shape
pub(crate) fn set_msghdr_name(msg: &mut msghdr, name: &SockAddr) {
    msg.msg_name = name.as_ptr() as *mut _;        // SockAddr::as_ptr(&self) -> *const _
    msg.msg_namelen = name.len();
}

SockAddr only has as_ptr(&self), so msg_name is always derived from a shared reference even
on the mutable path. The #[allow(clippy::needless_pass_by_ref_mut)] is a symptom of the same
thing: clippy correctly observes that the &mut is never used mutably on the Rust side, because
the only mutable use happens inside the kernel.

Reproduction

Miri cannot run recvmsg, so this test obtains the pointer through the crate's own code path
and only models the kernel's write. Dropped into src/lib.rs of 0.6.5:

#[cfg(all(test, unix, not(target_os = "redox")))]
mod f05_repro {
    use super::*;
    use std::mem::MaybeUninit;

    #[test]
    fn kernel_write_through_msg_name_is_ub() {
        let storage = unsafe { MaybeUninit::<SockAddrStorage>::zeroed().assume_init() };
        let mut addr =
            unsafe { SockAddr::new(storage, std::mem::size_of::<SockAddrStorage>() as _) };
        let mut buf = [MaybeUninit::new(0u8); 8];
        let mut bufs = [MaybeUninitSlice::new(&mut buf)];

        let hdr = MsgHdrMut::new().with_addr(&mut addr).with_buffers(&mut bufs);

        // what recvmsg(2) does: write the sender address into msg_name
        let name = hdr.inner.msg_name as *mut u8;
        unsafe { name.write(2u8) };

        let _ = addr.family();
    }
}

Stacked Borrows (cargo miri test --lib f05_repro):

error: Undefined Behavior: attempting a write access using <138399> at alloc...[0x0],
       but that tag only grants SharedReadOnly permission for this location
   --> src/lib.rs:762:18
help: <138399> was created by a SharedReadOnly retag at offsets [0x0..0x80]
   --> src/sockaddr.rs:253:9
    |
253 |         &self.storage as *const sockaddr_storage as *const SockAddrStorage

Tree Borrows (MIRIFLAGS=-Zmiri-tree-borrows):

error: Undefined Behavior: write access through <134125> at alloc...[0x0] is forbidden
  = help: the accessed tag <134125> has state Frozen which forbids this child write access
help: the accessed tag <134125> was created here, in the initial state Frozen
   --> src/sockaddr.rs:253:9

Both models point at SockAddr::as_ptr as the origin.

Reachable with no unsafe in user code: Socket::recvmsg(&self, msg: &mut MsgHdrMut, flags) is a
safe pub fn, and so are MsgHdrMut::new, with_addr and with_buffers.

I have not observed a miscompilation from this; current rustc keeps the accesses. I am
reporting it as unsoundness.

Not covered by existing reports

• RUSTSEC-2020-0079 is the old SocketAddr memory-layout assumption — unrelated.
• There are no Stacked Borrows or Miri issues on this repo (I searched the tracker), and Add Socket::{send,recv}msg and MsgHdr(Mut) #447 is
  where MsgHdrMut was introduced to address the &MsgHdr case.

Suggested fix

Give SockAddr a mutable accessor and a mutable setter, and use it on the MsgHdrMut path:

// src/sockaddr.rs, next to as_ptr
pub(crate) fn as_mut_ptr(&mut self) -> *mut SockAddrStorage {
    &mut self.storage as *mut sockaddr_storage as *mut SockAddrStorage
}

// src/sys/unix.rs (and the matching shape in src/sys/windows.rs)
pub(crate) fn set_msghdr_name_mut(msg: &mut msghdr, name: &mut SockAddr) {
    msg.msg_namelen = name.len();
    msg.msg_name = name.as_mut_ptr() as *mut _;
}

// src/lib.rs — the clippy allow can go away too
pub fn with_addr(mut self, addr: &'addr mut SockAddr) -> Self {
    sys::set_msghdr_name_mut(&mut self.inner, addr);
    self
}

MsgHdr::with_addr (the send path) keeps using set_msghdr_name, which is correct there since
sendmsg only reads.

I applied exactly this to a local copy of 0.6.5:

$ cargo miri test --lib f05_repro                                 # Stacked Borrows
test result: ok. 1 passed; 0 failed
$ MIRIFLAGS=-Zmiri-tree-borrows cargo miri test --lib f05_repro   # Tree Borrows
test result: ok. 1 passed; 0 failed
$ cargo test --lib
test result: ok. 11 passed; 0 failed

I only verified the Unix side; src/sys/windows.rs needs the same split. Happy to open a PR, and
happy to coordinate before anything is published.

Context

Found while developing a static checker for Rust's aliasing rules; I verified this one by hand with
Miri before reporting.