Merge pull request #1287 from microsoft/bugfix-inetstack-window-overflow

[inetstack] Bug Fix: Check overflow window
microsoft · May 22, 2024 · 8ef1126 · 8ef1126
2 parents c1c9c02 + 884d12a
commit 8ef1126
Show file tree

Hide file tree

Showing 3 changed files with 57 additions and 41 deletions.
diff --git a/src/rust/inetstack/protocols/tcp/active_open.rs b/src/rust/inetstack/protocols/tcp/active_open.rs
@@ -7,7 +7,6 @@
 
 use crate::{
     collections::async_queue::SharedAsyncQueue,
-    expect_ok,
     expect_some,
     inetstack::protocols::{
         arp::SharedArpPeer,
@@ -185,39 +184,32 @@ impl<N: NetworkRuntime> SharedActiveOpenSocket<N> {
 
         let (local_window_scale, remote_window_scale): (u32, u8) = match remote_window_scale {
             Some(remote_window_scale) => {
-                let local: u32 = if self.tcp_config.get_window_scale() > 14 {
-                    warn!("local windows scale larger than 14 is incorrect, so setting to 14. See RFC 1323.");
-                    MAX_WINDOW_SCALE as u32
-                } else {
-                    self.tcp_config.get_window_scale() as u32
-                };
-                let remote: u8 = if remote_window_scale > 14 {
-                    warn!("remote windows scale larger than 14 is incorrect, so setting to 14. See RFC 1323.");
+                let remote: u8 = if remote_window_scale as usize > MAX_WINDOW_SCALE {
+                    warn!(
+                        "remote windows scale larger than {:?} is incorrect, so setting to {:?}. See RFC 1323.",
+                        MAX_WINDOW_SCALE, MAX_WINDOW_SCALE
+                    );
                     MAX_WINDOW_SCALE as u8
                 } else {
                     remote_window_scale
                 };
-                (local, remote)
+                (self.tcp_config.get_window_scale() as u32, remote)
             },
             None => (0, 0),
         };
 
-        let rx_window_size: u32 = expect_ok!(
-            expect_some!(
-                (self.tcp_config.get_receive_window_size() as u32).checked_shl(local_window_scale as u32),
-                "TODO: Window size overflow"
-            )
-            .try_into(),
-            "TODO: Window size overflow"
+        // Expect is safe here because the receive window size is a 16-bit unsigned integer and MAX_WINDOW_SCALE is 14,
+        // so it is impossible to overflow the 32-bit unsigned int.
+        debug_assert!((local_window_scale as usize) <= MAX_WINDOW_SCALE);
+        let rx_window_size: u32 = expect_some!(
+            (self.tcp_config.get_receive_window_size() as u32).checked_shl(local_window_scale as u32),
+            "Window size overflow"
         );
-
-        let tx_window_size: u32 = expect_ok!(
-            expect_some!(
-                (header.window_size as u32).checked_shl(remote_window_scale as u32),
-                "TODO: Window size overflow"
-            )
-            .try_into(),
-            "TODO: Window size overflow"
+        // Expect is safe here because the window size is a 16-bit unsigned integer and MAX_WINDOW_SCALE is 14, so it is impossible to overflow the 32-bit
+        debug_assert!((remote_window_scale as usize) <= MAX_WINDOW_SCALE);
+        let tx_window_size: u32 = expect_some!(
+            (header.window_size as u32).checked_shl(remote_window_scale as u32),
+            "Window size overflow"
         );
 
         info!("Window sizes: local {}, remote {}", rx_window_size, tx_window_size);

diff --git a/src/rust/inetstack/protocols/tcp/passive_open.rs b/src/rust/inetstack/protocols/tcp/passive_open.rs
@@ -10,7 +10,6 @@ use crate::{
         AsyncQueue,
         SharedAsyncQueue,
     },
-    expect_ok,
     expect_some,
     inetstack::protocols::{
         arp::SharedArpPeer,
@@ -23,8 +22,10 @@ use crate::{
         tcp::{
             constants::FALLBACK_MSS,
             established::{
-                congestion_control,
-                congestion_control::CongestionControl,
+                congestion_control::{
+                    self,
+                    CongestionControl,
+                },
                 EstablishedSocket,
             },
             isn_generator::IsnGenerator,
@@ -42,6 +43,7 @@ use crate::{
         memory::DemiBuffer,
         network::{
             config::TcpConfig,
+            consts::MAX_WINDOW_SCALE,
             types::MacAddress,
             NetworkRuntime,
         },
@@ -404,21 +406,34 @@ impl<N: NetworkRuntime> SharedPassiveSocket<N> {
             return Err(Fail::new(EBADMSG, "invalid SYN+ACK seq num"));
         }
 
-        let (local_window_scale, remote_window_scale) = match remote_window_scale {
-            Some(w) => (self.tcp_config.get_window_scale() as u32, w),
+        // Calculate the window.
+        let (local_window_scale, remote_window_scale): (u32, u8) = match remote_window_scale {
+            Some(remote_window_scale) => {
+                if (remote_window_scale as usize) < MAX_WINDOW_SCALE {
+                    (self.tcp_config.get_window_scale() as u32, remote_window_scale)
+                } else {
+                    warn!(
+                        "remote windows scale larger than {:?} is incorrect, so setting to {:?}. See RFC 1323.",
+                        MAX_WINDOW_SCALE, MAX_WINDOW_SCALE
+                    );
+                    (self.tcp_config.get_window_scale() as u32, MAX_WINDOW_SCALE as u8)
+                }
+            },
             None => (0, 0),
         };
-        let remote_window_size = expect_ok!(
-            expect_some!(
-                (header_window_size as u32).checked_shl(remote_window_scale as u32),
-                "TODO: Window size overflow"
-            )
-            .try_into(),
-            "TODO: Window size overflow"
+
+        // Expect is safe here because the window size is a 16-bit unsigned integer and MAX_WINDOW_SCALE is 14, so it is impossible to overflow the 32-bit
+        debug_assert!((remote_window_scale as usize) <= MAX_WINDOW_SCALE);
+        let remote_window_size: u32 = expect_some!(
+            (header_window_size as u32).checked_shl(remote_window_scale as u32),
+            "Window size overflow"
         );
-        let local_window_size = expect_some!(
-            (self.tcp_config.get_receive_window_size() as u32).checked_shl(local_window_scale as u32),
-            "TODO: Window size overflow"
+        // Expect is safe here because the receive window size is a 16-bit unsigned integer and MAX_WINDOW_SCALE is 14,
+        // so it is impossible to overflow the 32-bit unsigned int.
+        debug_assert!((local_window_scale as usize) <= MAX_WINDOW_SCALE);
+        let local_window_size: u32 = expect_some!(
+            (self.tcp_config.get_receive_window_size() as u32).checked_shl(local_window_scale),
+            "Window size overflow"
         );
         info!(
             "Window sizes: local {}, remote {}",

diff --git a/src/rust/runtime/network/config/tcp.rs b/src/rust/runtime/network/config/tcp.rs
@@ -8,6 +8,7 @@
 use crate::runtime::network::consts::{
     DEFAULT_MSS,
     MAX_MSS,
+    MAX_WINDOW_SCALE,
     MIN_MSS,
     TCP_ACK_DELAY_TIMEOUT,
     TCP_HANDSHAKE_TIMEOUT,
@@ -71,7 +72,15 @@ impl TcpConfig {
             options = options.set_receive_window_size(value);
         }
         if let Some(value) = window_scale {
-            options = options.set_window_scale(value);
+            if (value as usize) < MAX_WINDOW_SCALE {
+                options = options.set_window_scale(value);
+            } else {
+                warn!(
+                    "remote windows scale larger than {:?} is incorrect, so setting to {:?}. See RFC 1323.",
+                    MAX_WINDOW_SCALE, MAX_WINDOW_SCALE
+                );
+                options = options.set_window_scale(MAX_WINDOW_SCALE as u8);
+            }
         }
         if let Some(value) = ack_delay_timeout {
             options = options.set_ack_delay_timeout(value);