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

Fixes recvm(m)sg blindly assuming correct initialization of received address #2249

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
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 changelog/2249.fixed.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fixed `recvm(m)sg` not checking the address family field of the received address.
21 changes: 11 additions & 10 deletions src/sys/socket/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1718,15 +1718,15 @@ where

// as long as we are not reading past the index writen by recvmmsg - address
// will be initialized
let address = unsafe { self.rmm.addresses[self.current_index].assume_init() };
let address = self.rmm.addresses[self.current_index];

self.current_index += 1;
Some(unsafe {
read_mhdr(
mmsghdr.msg_hdr,
mmsghdr.msg_len as isize,
self.rmm.msg_controllen,
address,
&address,
)
})
}
Expand Down Expand Up @@ -1778,7 +1778,7 @@ unsafe fn read_mhdr<'a, 'i, S>(
mhdr: msghdr,
r: isize,
msg_controllen: usize,
mut address: S,
address: &mem::MaybeUninit<S>,
) -> RecvMsg<'a, 'i, S>
where S: SockaddrLike
{
Expand All @@ -1798,15 +1798,16 @@ unsafe fn read_mhdr<'a, 'i, S>(
}
};

// Ignore errors if this socket address has statically-known length
//
// This is to ensure that unix socket addresses have their length set appropriately.
let _ = unsafe { address.set_length(mhdr.msg_namelen as usize) };
let addr_len = mhdr.msg_namelen;

let address = unsafe {
S::from_raw(address.as_ptr().cast(), Some(addr_len))
Copy link
Member

Choose a reason for hiding this comment

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

Are you aware that this creates a data copy? The original code was carefully designed to avoid such a thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it creates a copy, but so does recvfrom (currently, on the master branch):

Ok((ret, T::from_raw(addr.assume_init().as_ptr(), Some(len))))

I feel like in a future PR we should make the address field of RecvMsg private and instead add a getter that lazily performs the checks and the copy, like this:

pub fn address(&self) -> Option<S> {
    S::from_raw(self.address.as_ptr(), len)
}

Copy link
Member

Choose a reason for hiding this comment

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

Why does it matter that some other function also performs a data copy? Are you pointing that out to suggest that we improve it? I'm requesting that you not add an additional data copy to recvmsg.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah okay yeah, that makes sense, I think I misunderstood you before. I'll change the signature of read_mhdr back to expecting a pointer, and perform the conversion there.

};

RecvMsg {
bytes: r as usize,
cmsghdr,
address: Some(address),
address,
flags: MsgFlags::from_bits_truncate(mhdr.msg_flags),
mhdr,
iobufs: std::marker::PhantomData,
Expand Down Expand Up @@ -1936,7 +1937,7 @@ pub fn recvmsg<'a, 'outer, 'inner, S>(fd: RawFd, iov: &'outer mut [IoSliceMut<'i

let r = Errno::result(ret)?;

Ok(unsafe { read_mhdr(mhdr, r, msg_controllen, address.assume_init()) })
Ok(unsafe { read_mhdr(mhdr, r, msg_controllen, &address) })
}
}

Expand Down Expand Up @@ -2112,7 +2113,7 @@ pub fn recvfrom<T: SockaddrLike>(
&mut len as *mut socklen_t,
))? as usize;

Ok((ret, T::from_raw(addr.assume_init().as_ptr(), Some(len))))
Ok((ret, T::from_raw(addr.as_ptr().cast(), Some(len))))
asomers marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down
84 changes: 84 additions & 0 deletions test/sys/test_socket.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2915,3 +2915,87 @@ fn can_open_routing_socket() {
socket(AddressFamily::Route, SockType::Raw, SockFlag::empty(), None)
.expect("Failed to open routing socket");
}

#[test]
fn test_recvmsg_wrong_addr_type() {
use std::io::{IoSlice, IoSliceMut};

use nix::sys::socket::*;

let send = socket(
AddressFamily::Inet,
SockType::Datagram,
SockFlag::empty(),
SockProtocol::Udp,
)
.unwrap();

let recv = socket(
AddressFamily::Inet,
SockType::Datagram,
SockFlag::empty(),
SockProtocol::Udp,
)
.unwrap();

let addr = "127.0.0.1:6803".parse::<SockaddrIn>().unwrap();

bind(recv.as_raw_fd(), &addr).unwrap();

sendmsg(
send.as_raw_fd(),
&[IoSlice::new(&[0x69; 42][..])],
&[],
MsgFlags::empty(),
Some(&addr),
)
.unwrap();

// To make apple happy
let mut buf = [0u8; 42];
let mut iov = [IoSliceMut::new(&mut buf)];

let r = recvmsg::<UnixAddr>(
recv.as_raw_fd(),
&mut iov,
None,
MsgFlags::empty(),
)
.unwrap();

assert!(r.address.is_none());
}

#[test]
fn test_recvfrom_wrong_addr_type() {
use nix::sys::socket::*;

let send = socket(
AddressFamily::Inet,
SockType::Datagram,
SockFlag::empty(),
SockProtocol::Udp,
)
.unwrap();

let recv = socket(
AddressFamily::Inet,
SockType::Datagram,
SockFlag::empty(),
SockProtocol::Udp,
)
.unwrap();

let addr = "127.0.0.1:6804".parse::<SockaddrIn>().unwrap();

bind(recv.as_raw_fd(), &addr).unwrap();

sendto(send.as_raw_fd(), &[0x69; 42], &addr, MsgFlags::empty()).unwrap();

// To make apple happy
let mut buf = [0u8; 42];

let (_, addr) = recvfrom::<UnixAddr>(recv.as_raw_fd(), &mut buf).unwrap();

assert!(addr.is_none());
}