Skip to content
This repository has been archived by the owner on Oct 26, 2022. It is now read-only.

Commit

Permalink
Fix UB from unsafe uninitialized buffer slices
Browse files Browse the repository at this point in the history
Creating &mut [u8] (and &[u8]) for unitialized memory is undefined
behaviour even when not actually reading the data.

Pass &mut [MaybeUninit<u8>] instead (the recv implementations should
even fulfill the bytes::buf::UninitSlice requirement of not writing
uninitialized data (to the potentially already initialized memory!).

This also bumps netlink-sys; the other crates already got bumped.
  • Loading branch information
stbuehler committed Nov 13, 2021
1 parent a291cf4 commit 3d346cb
Show file tree
Hide file tree
Showing 10 changed files with 24 additions and 21 deletions.
2 changes: 1 addition & 1 deletion ethtool/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ netlink-packet-core = "0.3.0"
netlink-packet-generic = "0.2.0"
netlink-packet-utils = "0.4.1"
netlink-proto = { default-features = false, version = "0.8.0" }
netlink-sys = "0.7.0"
netlink-sys = "0.8.0"
thiserror = "1.0.29"
tokio = { version = "1.0.1", features = ["rt"], optional = true}

Expand Down
2 changes: 1 addition & 1 deletion netlink-packet-generic/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,4 @@ netlink-packet-core = "0.3"
netlink-packet-utils = "0.4"

[dev-dependencies]
netlink-sys = { path = "../netlink-sys", version = "0.7" }
netlink-sys = { path = "../netlink-sys", version = "0.8" }
3 changes: 1 addition & 2 deletions netlink-packet-generic/tests/query_family_id.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,7 @@ fn query_family_id() {

socket.send(&txbuf, 0).unwrap();

let mut rxbuf = vec![0u8; 2048];
socket.recv(&mut rxbuf, 0).unwrap();
let (rxbuf, _addr) = socket.recv_from_full().unwrap();
let rx_packet = <NetlinkMessage<GenlMessage<GenlCtrl>>>::deserialize(&rxbuf).unwrap();

if let NetlinkPayload::InnerMessage(genlmsg) = rx_packet.payload {
Expand Down
2 changes: 1 addition & 1 deletion netlink-packet-route/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ name = "dump_links"
criterion = "0.3.0"
pcap-file = "1.1.1"
lazy_static = "1.4.0"
netlink-sys = "0.7"
netlink-sys = "0.8"

[[bench]]
name = "link_message"
Expand Down
2 changes: 1 addition & 1 deletion netlink-packet-sock-diag/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,4 @@ smallvec = "1.4.2"

[dev-dependencies]
lazy_static = "1.4.0"
netlink-sys = "0.7"
netlink-sys = "0.8"
2 changes: 1 addition & 1 deletion netlink-proto/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ log = "0.4.8"
futures = "0.3"
tokio = { version = "1.0", default-features = false, features = ["io-util"] }
netlink-packet-core = "0.3"
netlink-sys = { default-features = false, version = "0.7" }
netlink-sys = { default-features = false, version = "0.8" }

[features]
default = ["tokio_socket"]
Expand Down
3 changes: 2 additions & 1 deletion netlink-proto/src/framed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use std::{
fmt::Debug,
io,
marker::PhantomData,
mem::MaybeUninit,
pin::Pin,
slice,
task::{Context, Poll},
Expand Down Expand Up @@ -63,7 +64,7 @@ where
// memory during a recv so it's fine to turn &mut
// [<MaybeUninitialized<u8>>] into &mut[u8]
let bytes = reader.chunk_mut();
let bytes = slice::from_raw_parts_mut(bytes.as_mut_ptr(), bytes.len());
let bytes = slice::from_raw_parts_mut(bytes.as_mut_ptr() as *mut MaybeUninit<u8>, bytes.len());
match ready!(socket.poll_recv_from(cx, bytes)) {
Ok((n, addr)) => {
reader.advance_mut(n);
Expand Down
2 changes: 1 addition & 1 deletion netlink-sys/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
[package]
authors = ["Corentin Henry <corentinhenry@gmail.com>"]
name = "netlink-sys"
version = "0.7.0"
version = "0.8.0" # TODO: drop this comment - already bumped version for trait changes
edition = "2018"

homepage = "https://github.com/little-dude/netlink"
Expand Down
20 changes: 11 additions & 9 deletions netlink-sys/src/socket.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::{
io::{Error, Result},
mem,
mem::{self, MaybeUninit},
os::unix::io::{AsRawFd, FromRawFd, RawFd},
};

Expand Down Expand Up @@ -224,7 +224,7 @@ impl Socket {
/// In datagram oriented protocols, `recv` and `recvfrom` receive normally only ONE datagram, but this seems not to
/// be always true for netlink sockets: with some protocols like `NETLINK_AUDIT`, multiple netlink packets can be
/// read with a single call.
pub fn recv_from(&self, buf: &mut [u8], flags: libc::c_int) -> Result<(usize, SocketAddr)> {
pub fn recv_from(&self, buf: &mut [MaybeUninit<u8>], flags: libc::c_int) -> Result<(usize, SocketAddr)> {
// Create an empty storage for the address. Note that Rust standard library create a
// sockaddr_storage so that it works for any address family, but here, we already know that
// we'll have a Netlink address, so we can create the appropriate storage.
Expand Down Expand Up @@ -273,7 +273,7 @@ impl Socket {

/// For a connected socket, `recv` reads a datagram from the socket. The sender is the remote peer the socket is
/// connected to (see [`Socket::connect`]). See also [`Socket::recv_from`]
pub fn recv(&self, buf: &mut [u8], flags: libc::c_int) -> Result<usize> {
pub fn recv(&self, buf: &mut [MaybeUninit<u8>], flags: libc::c_int) -> Result<usize> {
let buf_ptr = buf.as_mut_ptr() as *mut libc::c_void;
let buf_len = buf.len() as libc::size_t;

Expand All @@ -288,14 +288,16 @@ impl Socket {
/// buffer passed as argument, this method always reads a whole message, no matter its size.
pub fn recv_from_full(&self) -> Result<(Vec<u8>, SocketAddr)> {
// Peek
let mut buf = Vec::<u8>::new();
let (rlen, _) = self.recv_from(&mut buf, libc::MSG_PEEK | libc::MSG_TRUNC)?;
let (peek_len, _) = self.recv_from(&mut [], libc::MSG_PEEK | libc::MSG_TRUNC)?;

// Receive
let mut buf = vec![0; rlen as usize];
let (_, addr) = self.recv_from(&mut buf, 0)?;

Ok((buf, addr))
let mut buf: Vec<u8> = Vec::with_capacity(peek_len);
unsafe {
let (rlen, addr) = self.recv_from(std::slice::from_raw_parts_mut(buf.as_mut_ptr() as *mut MaybeUninit<u8>, peek_len), 0)?;
assert_eq!(rlen, peek_len);
buf.set_len(peek_len);
Ok((buf, addr))
}
}

/// Send the given buffer `buf` to the remote peer with address `addr`. The supported flags are the `MSG_*` values
Expand Down
7 changes: 4 additions & 3 deletions netlink-sys/src/tokio.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use std::{
io,
mem::MaybeUninit,
os::unix::io::{AsRawFd, FromRawFd, RawFd},
task::{Context, Poll},
};
Expand Down Expand Up @@ -71,7 +72,7 @@ impl TokioSocket {
}
}

pub async fn recv(&mut self, buf: &mut [u8]) -> io::Result<usize> {
pub async fn recv(&mut self, buf: &mut [MaybeUninit<u8>]) -> io::Result<usize> {
poll_fn(|cx| loop {
// Check if the socket is readable. If not,
// AsyncFd::poll_read_ready would have arranged for the
Expand All @@ -87,7 +88,7 @@ impl TokioSocket {
.await
}

pub async fn recv_from(&mut self, buf: &mut [u8]) -> io::Result<(usize, SocketAddr)> {
pub async fn recv_from(&mut self, buf: &mut [MaybeUninit<u8>]) -> io::Result<(usize, SocketAddr)> {
poll_fn(|cx| self.poll_recv_from(cx, buf)).await
}

Expand All @@ -98,7 +99,7 @@ impl TokioSocket {
pub fn poll_recv_from(
&mut self,
cx: &mut Context,
buf: &mut [u8],
buf: &mut [MaybeUninit<u8>],
) -> Poll<io::Result<(usize, SocketAddr)>> {
loop {
trace!("poll_recv_from called");
Expand Down

0 comments on commit 3d346cb

Please sign in to comment.