diff --git a/kernel/src/net/socket/netlink/message/attr/mod.rs b/kernel/src/net/socket/netlink/message/attr/mod.rs index 7afdd6a94..9227b5592 100644 --- a/kernel/src/net/socket/netlink/message/attr/mod.rs +++ b/kernel/src/net/socket/netlink/message/attr/mod.rs @@ -51,9 +51,41 @@ pub struct CAttrHeader { } impl CAttrHeader { + /// Creates from the type and the payload length. + fn from_payload_len(type_: u16, payload_len: usize) -> Self { + let total_len = payload_len + size_of::(); + debug_assert!(total_len <= u16::MAX as usize); + + Self { + len: total_len as u16, + type_, + } + } + + /// Returns the type of the attribute. pub fn type_(&self) -> u16 { self.type_ & ATTRIBUTE_TYPE_MASK } + + /// Returns the payload length (excluding padding). + pub fn payload_len(&self) -> usize { + self.len as usize - size_of::() + } + + /// Returns the total length of the attribute (header + payload, excluding padding). + pub fn total_len(&self) -> usize { + self.len as usize + } + + /// Returns the total length of the attribute (header + payload, including padding). + pub fn total_len_with_padding(&self) -> usize { + (self.len as usize).align_up(NLMSG_ALIGN) + } + + /// Returns the length of the padding bytes. + pub fn padding_len(&self) -> usize { + self.total_len_with_padding() - self.total_len() + } } const IS_NESTED_MASK: u16 = 1u16 << 15; @@ -68,49 +100,65 @@ pub trait Attribute: Debug + Send + Sync { /// Returns the byte representation of the payload. fn payload_as_bytes(&self) -> &[u8]; - /// Returns the payload length (excluding padding). - fn payload_len(&self) -> usize { - self.payload_as_bytes().len() - } - - /// Returns the total length of the attribute (header + payload, excluding padding). - fn total_len(&self) -> usize { - core::mem::size_of::() + self.payload_len() - } - /// Returns the total length of the attribute (header + payload, including padding). fn total_len_with_padding(&self) -> usize { - self.total_len().align_up(NLMSG_ALIGN) - } + // We don't care the attribute type when calculating the attribute length. + const DUMMY_TYPE: u16 = 0; - /// Returns the length of the padding bytes. - fn padding_len(&self) -> usize { - self.total_len_with_padding() - self.total_len() + CAttrHeader::from_payload_len(DUMMY_TYPE, self.payload_as_bytes().len()) + .total_len_with_padding() } /// Reads the attribute from the `reader`. - fn read_from(reader: &mut dyn MultiRead) -> Result + /// + /// This method may return a `None` if the attribute is not recognized. In that case, however, + /// it must still skip the payload length (excluding padding), as if the attribute were parsed + /// properly. + fn read_from(header: &CAttrHeader, reader: &mut dyn MultiRead) -> Result> where Self: Sized; /// Reads all attributes from the reader. /// - /// The cumulative length of the read attributes must not exceed total_len. + /// The cumulative length of the read attributes must not exceed `total_len`. fn read_all_from(reader: &mut dyn MultiRead, mut total_len: usize) -> Result> where Self: Sized, { let mut res = Vec::new(); - while total_len > 0 { - let attr = Self::read_from(reader)?; - total_len -= attr.total_len(); + // Below, we're performing strict validation. Although Linux tends to perform strict + // validation for new netlink message consumers, it may allow fewer or no validations for + // legacy consumers. See + // for + // more details. - let padding_len = attr.padding_len().min(total_len); + while total_len > 0 { + // Validate the remaining length for the attribute header length. + if total_len < size_of::() { + return_errno_with_message!(Errno::EINVAL, "the reader length is too small"); + } + + // Read and validate the attribute header. + let header = reader.read_val_opt::()?.unwrap(); + if header.total_len() < size_of::() { + return_errno_with_message!(Errno::EINVAL, "the attribute length is too small"); + } + + // Validate the remaining length for the attribute payload length. + total_len = total_len.checked_sub(header.total_len()).ok_or_else(|| { + Error::with_message(Errno::EINVAL, "the reader size is too small") + })?; + + // Read the attribute. + if let Some(attr) = Self::read_from(&header, reader)? { + res.push(attr); + } + + // Skip the padding bytes. + let padding_len = total_len.min(header.padding_len()); reader.skip_some(padding_len); total_len -= padding_len; - - res.push(attr); } Ok(res) @@ -118,15 +166,14 @@ pub trait Attribute: Debug + Send + Sync { /// Writes the attribute to the `writer`. fn write_to(&self, writer: &mut dyn MultiWrite) -> Result<()> { - let header = CAttrHeader { - type_: self.type_(), - len: self.total_len() as u16, - }; + let type_ = self.type_(); + let payload = self.payload_as_bytes(); + let header = CAttrHeader::from_payload_len(type_, payload.len()); writer.write_val_trunc(&header)?; - writer.write(&mut VmReader::from(self.payload_as_bytes()))?; + writer.write(&mut VmReader::from(payload))?; - let padding_len = self.padding_len(); + let padding_len = header.padding_len(); writer.skip_some(padding_len); Ok(()) diff --git a/kernel/src/net/socket/netlink/message/attr/noattr.rs b/kernel/src/net/socket/netlink/message/attr/noattr.rs index 6b83bcb87..d80f95438 100644 --- a/kernel/src/net/socket/netlink/message/attr/noattr.rs +++ b/kernel/src/net/socket/netlink/message/attr/noattr.rs @@ -1,6 +1,6 @@ // SPDX-License-Identifier: MPL-2.0 -use super::Attribute; +use super::{Attribute, CAttrHeader}; use crate::{prelude::*, util::MultiRead}; /// A special type indicates that a segment cannot have attributes. @@ -16,11 +16,14 @@ impl Attribute for NoAttr { match *self {} } - fn read_from(_reader: &mut dyn MultiRead) -> Result + fn read_from(header: &CAttrHeader, reader: &mut dyn MultiRead) -> Result> where Self: Sized, { - return_errno_with_message!(Errno::EINVAL, "`NoAttr` cannot be read"); + let payload_len = header.payload_len(); + reader.skip_some(payload_len); + + Ok(None) } fn read_all_from(_reader: &mut dyn MultiRead, _total_len: usize) -> Result> diff --git a/kernel/src/net/socket/netlink/message/segment/common.rs b/kernel/src/net/socket/netlink/message/segment/common.rs index c00a061fa..7ed3d6e5e 100644 --- a/kernel/src/net/socket/netlink/message/segment/common.rs +++ b/kernel/src/net/socket/netlink/message/segment/common.rs @@ -51,7 +51,7 @@ impl SegmentCommon { where Error: From<>::Error>, { - let (body, remain_len) = Body::read_from(&header, reader).unwrap(); + let (body, remain_len) = Body::read_from(&header, reader)?; let attrs = Attr::read_all_from(reader, remain_len)?; Ok(Self { diff --git a/kernel/src/net/socket/netlink/route/bound.rs b/kernel/src/net/socket/netlink/route/bound.rs index 87debf64e..1c5fab3e6 100644 --- a/kernel/src/net/socket/netlink/route/bound.rs +++ b/kernel/src/net/socket/netlink/route/bound.rs @@ -56,22 +56,20 @@ impl datagram_common::Bound for BoundNetlinkRoute { ); } - let mut nlmsg = { - let sum_lens = reader.sum_lens(); + let sum_lens = reader.sum_lens(); - match RtnlMessage::read_from(reader) { - Ok(nlmsg) => nlmsg, - Err(e) if e.error() == Errno::EFAULT => { - // EFAULT indicates an error occurred while copying data from user space, - // and this error should be returned back to user space. - return Err(e); - } - Err(e) => { - // Errors other than EFAULT indicate a failure in parsing the netlink message. - // These errors should be silently ignored. - warn!("failed to send netlink message: {:?}", e); - return Ok(sum_lens); - } + let mut nlmsg = match RtnlMessage::read_from(reader) { + Ok(nlmsg) => nlmsg, + Err(e) if e.error() == Errno::EFAULT => { + // EFAULT indicates an error occurred while copying data from user space, + // and this error should be returned back to user space. + return Err(e); + } + Err(e) => { + // Errors other than EFAULT indicate a failure in parsing the netlink message. + // These errors should be silently ignored. + warn!("failed to send netlink message: {:?}", e); + return Ok(sum_lens); } }; @@ -88,7 +86,7 @@ impl datagram_common::Bound for BoundNetlinkRoute { get_netlink_route_kernel().request(&nlmsg, local_port); - Ok(nlmsg.total_len()) + Ok(sum_lens) } fn try_recv( diff --git a/kernel/src/net/socket/netlink/route/message/attr/addr.rs b/kernel/src/net/socket/netlink/route/message/attr/addr.rs index c8d0d5f3d..9aeda0e8c 100644 --- a/kernel/src/net/socket/netlink/route/message/attr/addr.rs +++ b/kernel/src/net/socket/netlink/route/message/attr/addr.rs @@ -58,25 +58,41 @@ impl Attribute for AddrAttr { } } - fn read_from(reader: &mut dyn MultiRead) -> Result + fn read_from(header: &CAttrHeader, reader: &mut dyn MultiRead) -> Result> where Self: Sized, { - let header = reader.read_val_opt::()?.unwrap(); + let payload_len = header.payload_len(); // TODO: Currently, `IS_NET_BYTEORDER_MASK` and `IS_NESTED_MASK` are ignored. - let res = match AddrAttrClass::try_from(header.type_())? { - AddrAttrClass::ADDRESS => Self::Address(reader.read_val_opt()?.unwrap()), - AddrAttrClass::LOCAL => Self::Local(reader.read_val_opt()?.unwrap()), - AddrAttrClass::LABEL => Self::Label(reader.read_cstring_with_max_len(IFNAME_SIZE)?), - class => { - // FIXME: Netlink should ignore all unknown attributes. - // See the reference in `LinkAttr::read_from`. + let Ok(class) = AddrAttrClass::try_from(header.type_()) else { + // Unknown attributes should be ignored. + // Reference: . + reader.skip_some(payload_len); + return Ok(None); + }; + + let res = match (class, payload_len) { + (AddrAttrClass::ADDRESS, 4) => { + Self::Address(reader.read_val_opt::<[u8; 4]>()?.unwrap()) + } + (AddrAttrClass::LOCAL, 4) => Self::Local(reader.read_val_opt::<[u8; 4]>()?.unwrap()), + (AddrAttrClass::LABEL, 1..=IFNAME_SIZE) => { + Self::Label(reader.read_cstring_with_max_len(payload_len)?) + } + + (AddrAttrClass::ADDRESS | AddrAttrClass::LOCAL | AddrAttrClass::LABEL, _) => { + warn!("address attribute `{:?}` contains invalid payload", class); + return_errno_with_message!(Errno::EINVAL, "the address attribute is invalid"); + } + + (_, _) => { warn!("address attribute `{:?}` is not supported", class); - return_errno_with_message!(Errno::EINVAL, "unsupported address attribute"); + reader.skip_some(payload_len); + return Ok(None); } }; - Ok(res) + Ok(Some(res)) } } diff --git a/kernel/src/net/socket/netlink/route/message/attr/link.rs b/kernel/src/net/socket/netlink/route/message/attr/link.rs index 9fcd969cd..7ffc7a3dd 100644 --- a/kernel/src/net/socket/netlink/route/message/attr/link.rs +++ b/kernel/src/net/socket/netlink/route/message/attr/link.rs @@ -118,29 +118,52 @@ impl Attribute for LinkAttr { } } - fn read_from(reader: &mut dyn MultiRead) -> Result + fn read_from(header: &CAttrHeader, reader: &mut dyn MultiRead) -> Result> where Self: Sized, { - let header = reader.read_val_opt::()?.unwrap(); + let payload_len = header.payload_len(); // TODO: Currently, `IS_NET_BYTEORDER_MASK` and `IS_NESTED_MASK` are ignored. - let res = match LinkAttrClass::try_from(header.type_())? { - LinkAttrClass::IFNAME => Self::Name(reader.read_cstring_with_max_len(IFNAME_SIZE)?), - LinkAttrClass::MTU => Self::Mtu(reader.read_val_opt()?.unwrap()), - LinkAttrClass::TXQLEN => Self::TxqLen(reader.read_val_opt()?.unwrap()), - LinkAttrClass::LINKMODE => Self::LinkMode(reader.read_val_opt()?.unwrap()), - LinkAttrClass::EXT_MASK => Self::ExtMask(reader.read_val_opt()?.unwrap()), - class => { - // FIXME: Netlink should ignore all unknown attributes. - // But how to decide the payload type if the class is unknown? - // Reference: https://docs.kernel.org/userspace-api/netlink/intro.html#unknown-attributes + let Ok(class) = LinkAttrClass::try_from(header.type_()) else { + // Unknown attributes should be ignored. + // Reference: . + reader.skip_some(payload_len); + return Ok(None); + }; + + let res = match (class, payload_len) { + (LinkAttrClass::IFNAME, 1..=IFNAME_SIZE) => { + Self::Name(reader.read_cstring_with_max_len(payload_len)?) + } + (LinkAttrClass::MTU, 4) => Self::Mtu(reader.read_val_opt::()?.unwrap()), + (LinkAttrClass::TXQLEN, 4) => Self::TxqLen(reader.read_val_opt::()?.unwrap()), + (LinkAttrClass::LINKMODE, 1) => Self::LinkMode(reader.read_val_opt::()?.unwrap()), + (LinkAttrClass::EXT_MASK, 4) => { + const { assert!(size_of::() == 4) }; + Self::ExtMask(reader.read_val_opt::()?.unwrap()) + } + + ( + LinkAttrClass::IFNAME + | LinkAttrClass::MTU + | LinkAttrClass::TXQLEN + | LinkAttrClass::LINKMODE + | LinkAttrClass::EXT_MASK, + _, + ) => { + warn!("link attribute `{:?}` contains invalid payload", class); + return_errno_with_message!(Errno::EINVAL, "the link attribute is invalid"); + } + + (_, _) => { warn!("link attribute `{:?}` is not supported", class); - return_errno_with_message!(Errno::EINVAL, "unsupported link attribute"); + reader.skip_some(payload_len); + return Ok(None); } }; - Ok(res) + Ok(Some(res)) } } diff --git a/test/apps/network/netlink_route.c b/test/apps/network/netlink_route.c index 41ae985aa..9a80e62f1 100644 --- a/test/apps/network/netlink_route.c +++ b/test/apps/network/netlink_route.c @@ -152,6 +152,8 @@ char buffer[BUFFER_SIZE]; struct nl_req { struct nlmsghdr hdr; struct ifaddrmsg ifa; + struct nlattr ahdr; + char abuf[4]; }; FN_TEST(get_addr_error) @@ -184,41 +186,40 @@ FN_TEST(get_addr_error) ((struct nlmsgerr *)NLMSG_DATA(buffer))->error == -EOPNOTSUPP); + int found_new_addr; +#define TEST_KERNEL_RESPONSE \ + found_new_addr = 0; \ + while (1) { \ + size_t recv_len = \ + TEST_SUCC(recv(sock_fd, buffer, BUFFER_SIZE, 0)); \ + \ + int found_done = TEST_SUCC(find_new_addr_until_done( \ + buffer, recv_len, &found_new_addr)); \ + \ + if (found_done != 0) { \ + break; \ + } \ + } + // 2. Invalid required index req.hdr.nlmsg_flags = NLM_F_REQUEST | NLM_F_DUMP | NLM_F_ACK; req.ifa.ifa_index = 9999; TEST_SUCC(sendmsg(sock_fd, &msg, 0)); - - int found_new_addr = 0; - while (1) { - size_t recv_len = - TEST_SUCC(recv(sock_fd, buffer, BUFFER_SIZE, 0)); - - int found_done = TEST_SUCC(find_new_addr_until_done( - buffer, recv_len, &found_new_addr)); - - if (found_done != 0) { - break; - } - } + TEST_KERNEL_RESPONSE; // 3. Invalid required family req.ifa.ifa_family = 255; req.ifa.ifa_index = 0; TEST_SUCC(sendmsg(sock_fd, &msg, 0)); + TEST_KERNEL_RESPONSE; - found_new_addr = 0; - while (1) { - size_t recv_len = - TEST_SUCC(recv(sock_fd, buffer, BUFFER_SIZE, 0)); - - int found_done = TEST_SUCC(find_new_addr_until_done( - buffer, recv_len, &found_new_addr)); - - if (found_done != 0) { - break; - } - } + // 4. Unknown attribute + req.ahdr.nla_type = 0xdeef; + req.ahdr.nla_len = sizeof(req.ahdr) + sizeof(req.abuf); + req.hdr.nlmsg_len = sizeof(req); + iov = (struct iovec){ &req, sizeof(req) }; + TEST_SUCC(sendmsg(sock_fd, &msg, 0)); + TEST_KERNEL_RESPONSE; TEST_SUCC(close(sock_fd)); }