Handle broken netlink attributes

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::<Self>();
+        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::<Self>()
+    }
+
+    /// 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::<CAttrHeader>() + 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.
+        CAttrHeader::from_payload_len(DUMMY_TYPE, self.payload_as_bytes().len())
-    fn padding_len(&self) -> usize {
+            .total_len_with_padding()
-        self.total_len_with_padding() - self.total_len()
     }
 
     /// Reads the attribute from the `reader`.
-    fn read_from(reader: &mut dyn MultiRead) -> Result<Self>
+    ///
+    /// 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<Option<Self>>
     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<Vec<Self>>
     where
         Self: Sized,
     {
         let mut res = Vec::new();
 
-        while total_len > 0 {
+        // Below, we're performing strict validation. Although Linux tends to perform strict
-            let attr = Self::read_from(reader)?;
+        // validation for new netlink message consumers, it may allow fewer or no validations for
-            total_len -= attr.total_len();
+        // legacy consumers. See
+        // <https://github.com/torvalds/linux/commit/8cb081746c031fb164089322e2336a0bf5b3070c> 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::<CAttrHeader>() {
+                return_errno_with_message!(Errno::EINVAL, "the reader length is too small");
+            }
+
+            // Read and validate the attribute header.
+            let header = reader.read_val_opt::<CAttrHeader>()?.unwrap();
+            if header.total_len() < size_of::<CAttrHeader>() {
+                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 {
+        let type_ = self.type_();
-            type_: self.type_(),
+        let payload = self.payload_as_bytes();
-            len: self.total_len() as u16,
-        };
 
+        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(())

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<Self>
+    fn read_from(header: &CAttrHeader, reader: &mut dyn MultiRead) -> Result<Option<Self>>
     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<Vec<Self>>

kernel/src/net/socket/netlink/message/segment/common.rs

@@ -51,7 +51,7 @@ impl<Body: SegmentBody, Attr: Attribute> SegmentCommon<Body, Attr> {
     where
         Error: From<<Body::CType as TryInto<Body>>::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 {

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) {
+        let mut nlmsg = match RtnlMessage::read_from(reader) {
-                Ok(nlmsg) => nlmsg,
+            Ok(nlmsg) => nlmsg,
-                Err(e) if e.error() == Errno::EFAULT => {
+            Err(e) if e.error() == Errno::EFAULT => {
-                    // EFAULT indicates an error occurred while copying data from user space,
+                // EFAULT indicates an error occurred while copying data from user space,
-                    // and this error should be returned back to user space.
+                // and this error should be returned back to user space.
-                    return Err(e);
+                return Err(e);
-                }
+            }
-                Err(e) => {
+            Err(e) => {
-                    // Errors other than EFAULT indicate a failure in parsing the netlink message.
+                // Errors other than EFAULT indicate a failure in parsing the netlink message.
-                    // These errors should be silently ignored.
+                // These errors should be silently ignored.
-                    warn!("failed to send netlink message: {:?}", e);
+                warn!("failed to send netlink message: {:?}", e);
-                    return Ok(sum_lens);
+                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(

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<Self>
+    fn read_from(header: &CAttrHeader, reader: &mut dyn MultiRead) -> Result<Option<Self>>
     where
         Self: Sized,
     {
-        let header = reader.read_val_opt::<CAttrHeader>()?.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_())? {
+        let Ok(class) = AddrAttrClass::try_from(header.type_()) else {
-            AddrAttrClass::ADDRESS => Self::Address(reader.read_val_opt()?.unwrap()),
+            // Unknown attributes should be ignored.
-            AddrAttrClass::LOCAL => Self::Local(reader.read_val_opt()?.unwrap()),
+            // Reference: <https://docs.kernel.org/userspace-api/netlink/intro.html#unknown-attributes>.
-            AddrAttrClass::LABEL => Self::Label(reader.read_cstring_with_max_len(IFNAME_SIZE)?),
+            reader.skip_some(payload_len);
-            class => {
+            return Ok(None);
-                // FIXME: Netlink should ignore all unknown attributes.
+        };
-                // See the reference in `LinkAttr::read_from`.
+
+        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))
     }
 }

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<Self>
+    fn read_from(header: &CAttrHeader, reader: &mut dyn MultiRead) -> Result<Option<Self>>
     where
         Self: Sized,
     {
-        let header = reader.read_val_opt::<CAttrHeader>()?.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_())? {
+        let Ok(class) = LinkAttrClass::try_from(header.type_()) else {
-            LinkAttrClass::IFNAME => Self::Name(reader.read_cstring_with_max_len(IFNAME_SIZE)?),
+            // Unknown attributes should be ignored.
-            LinkAttrClass::MTU => Self::Mtu(reader.read_val_opt()?.unwrap()),
+            // Reference: <https://docs.kernel.org/userspace-api/netlink/intro.html#unknown-attributes>.
-            LinkAttrClass::TXQLEN => Self::TxqLen(reader.read_val_opt()?.unwrap()),
+            reader.skip_some(payload_len);
-            LinkAttrClass::LINKMODE => Self::LinkMode(reader.read_val_opt()?.unwrap()),
+            return Ok(None);
-            LinkAttrClass::EXT_MASK => Self::ExtMask(reader.read_val_opt()?.unwrap()),
+        };
-            class => {
+
-                // FIXME: Netlink should ignore all unknown attributes.
+        let res = match (class, payload_len) {
-                // But how to decide the payload type if the class is unknown?
+            (LinkAttrClass::IFNAME, 1..=IFNAME_SIZE) => {
-                // Reference: https://docs.kernel.org/userspace-api/netlink/intro.html#unknown-attributes
+                Self::Name(reader.read_cstring_with_max_len(payload_len)?)
+            }
+            (LinkAttrClass::MTU, 4) => Self::Mtu(reader.read_val_opt::<u32>()?.unwrap()),
+            (LinkAttrClass::TXQLEN, 4) => Self::TxqLen(reader.read_val_opt::<u32>()?.unwrap()),
+            (LinkAttrClass::LINKMODE, 1) => Self::LinkMode(reader.read_val_opt::<u8>()?.unwrap()),
+            (LinkAttrClass::EXT_MASK, 4) => {
+                const { assert!(size_of::<RtExtFilter>() == 4) };
+                Self::ExtMask(reader.read_val_opt::<RtExtFilter>()?.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))
     }
 }
 

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));
-
+	TEST_KERNEL_RESPONSE;
-	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;
-		}
-	}
 
 	// 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;
+	// 4. Unknown attribute
-	while (1) {
+	req.ahdr.nla_type = 0xdeef;
-		size_t recv_len =
+	req.ahdr.nla_len = sizeof(req.ahdr) + sizeof(req.abuf);
-			TEST_SUCC(recv(sock_fd, buffer, BUFFER_SIZE, 0));
+	req.hdr.nlmsg_len = sizeof(req);
-
+	iov = (struct iovec){ &req, sizeof(req) };
-		int found_done = TEST_SUCC(find_new_addr_until_done(
+	TEST_SUCC(sendmsg(sock_fd, &msg, 0));
-			buffer, recv_len, &found_new_addr));
+	TEST_KERNEL_RESPONSE;
-
-		if (found_done != 0) {
-			break;
-		}
-	}
 
 	TEST_SUCC(close(sock_fd));
 }