[ofw] [Patch][ipoib][ipoib_NDIS6_CM] Fixing a bug when OID_GEN_NETWORK_LAYER_ADDRESSES contains bad data

[Smith, Stan]

Hello,
  Patch applied and when one attempts to set an IPoIB IF IPv4 address the ASSERT( p_net_addr_oid->AddressType == NDIS_PROTOCOL_ID_TCP_IP ); fires.
Immediately prior to the ASSERT() the code

+   p_net_addr_oid = (PNETWORK_ADDRESS)((uint8_t *)p_net_addr_oid +
+        FIELD_OFFSET(NETWORK_ADDRESS, Address) +
+        p_net_addr_oid->AddressLength) ;
This code makes no sense to me in that the original code did not advance the p_net_addr_oid pointer? Why now?
If fact, if the above code is removed, the code performs the desired result in that an IPv4 address can be set on an IPoIB IF without the ASSERT() firing?

Thoughts?

Additionally you likely did not want to use NETWORK_ADDRESS_LENGTH_IPX but wanted NETWORK_ADDRESS_LENGTH_IP  in the following

ASSERT ( p_net_addr_oid->AddressLength == NETWORK_ADDRESS_LENGTH_IPX );
stan.


________________________________
From: ofw-bounces at lists.openfabrics.org [mailto:ofw-bounces at lists.openfabrics.org] On Behalf Of Alex Naslednikov
Sent: Monday, August 23, 2010 6:39 AM
To: ofw at lists.openfabrics.org
Subject: [ofw] [Patch][ipoib][ipoib_NDIS6_CM] Fixing a bug when OID_GEN_NETWORK_LAYER_ADDRESSES contains bad data

Fixing the bug when NDIS sends OID_GEN_NETWORK_LAYER_ADDRESSES with the
list of new addresses with invalid formatting (happened when AddressCount =5)

NDIS sends NETWORK_ADDRESS_LIST structure, which contains an array of NETWORK_ADDRESS structures of variable size.
The calculation of the next address offset is based on AddressLength;
in a case when this field contains wrong data, one can get access violation error

Signed-off by: Alexander Naslednikov (xalex at mellanox.co.il)

Index: B:/users/xalex/MLNX_WinOF-2_1_2/ulp/ipoib/kernel/ipoib_driver.c
===================================================================
--- B:/users/xalex/MLNX_WinOF-2_1_2/ulp/ipoib/kernel/ipoib_driver.c (revision 6298)
+++ B:/users/xalex/MLNX_WinOF-2_1_2/ulp/ipoib/kernel/ipoib_driver.c (revision 6299)
@@ -2210,30 +2210,27 @@
    cl_vector_get_ptr( &p_adapter->ip_vector, idx );
   p_net_addr_oid = (PNETWORK_ADDRESS)p_net_addrs->Address;

-  for( i = 0; i < p_net_addrs->AddressCount; ++i, p_net_addr_oid =
-   (PNETWORK_ADDRESS)((uint8_t *)p_net_addr_oid +
-   FIELD_OFFSET(NETWORK_ADDRESS, Address) +
-   p_net_addr_oid->AddressLength) )
+  for( i = 0; i < p_net_addrs->AddressCount; ++i )
   {

-   if( p_net_addr_oid->AddressType != NDIS_PROTOCOL_ID_TCP_IP )
-   {
-    IPOIB_PRINT( TRACE_LEVEL_WARNING, IPOIB_DBG_OID,
-     ("Port %d OID_GEN_NETWORK_LAYER_ADDRESSES - Address %d is wrong type of 0x%.4X, "
-      "should be 0x%.4X\n", port_num, i, p_net_addr_oid->AddressType,
-      NDIS_PROTOCOL_ID_TCP_IP));
-    continue;
-   }
-
+   // Here we check that the data stored at 'AddressLength' field is valid;
+   // otherwise, it can lead to a memory violation (happened when AddressCount was > 1)
    if( p_net_addr_oid->AddressLength != NETWORK_ADDRESS_LENGTH_IP)
    {
-    IPOIB_PRINT( TRACE_LEVEL_WARNING, IPOIB_DBG_OID,
+    IPOIB_PRINT(TRACE_LEVEL_ERROR, IPOIB_DBG_ERROR,
      ("Port %d OID_GEN_NETWORK_LAYER_ADDRESSES - Address %d is wrong size of %d, "
       "should be %d\n", port_num, i, p_net_addr_oid->AddressLength,
       NETWORK_ADDRESS_LENGTH_IP));
-    continue;
+    ASSERT ( p_net_addr_oid->AddressLength == NETWORK_ADDRESS_LENGTH_IPX );
+    break;
    }
+
+   p_net_addr_oid = (PNETWORK_ADDRESS)((uint8_t *)p_net_addr_oid +
+        FIELD_OFFSET(NETWORK_ADDRESS, Address) +
+        p_net_addr_oid->AddressLength) ;

+   ASSERT( p_net_addr_oid->AddressType == NDIS_PROTOCOL_ID_TCP_IP );
+
    p_ip_addr = (PNETWORK_ADDRESS_IP)p_net_addr_oid->Address;
    if( !cl_memcmp( &p_ip_addr->in_addr,
     &p_addr_item->address.as_ulong, sizeof(ULONG) ) )
@@ -2273,36 +2270,37 @@
  /* Now look for new addresses */
  p_net_addr_oid = (NETWORK_ADDRESS *)p_net_addrs->Address;
  idx = 0;
- for( i = 0; i < p_net_addrs->AddressCount; i++, p_net_addr_oid =
-  (PNETWORK_ADDRESS)((uint8_t *)p_net_addr_oid +
-  FIELD_OFFSET(NETWORK_ADDRESS, Address) + p_net_addr_oid->AddressLength) )
+
+ for( i = 0; i < p_net_addrs->AddressCount; ++i )
  {

-  if( p_net_addr_oid->AddressType != NDIS_PROTOCOL_ID_TCP_IP )
-  {
-   IPOIB_PRINT(TRACE_LEVEL_INFORMATION, IPOIB_DBG_OID,
-    ("Port %d OID_GEN_NETWORK_LAYER_ADDRESSES - Address %d is wrong type of 0x%.4X, "
-     "should be 0x%.4X\n", port_num, i, p_net_addr_oid->AddressType,
-     NDIS_PROTOCOL_ID_TCP_IP));
-   continue;
-  }
-
+  // Here we check that the data stored at 'AddressLength' field is valid;
+  // otherwise, it can lead to a memory violation (happened when AddressCount was > 1)
   if( p_net_addr_oid->AddressLength != NETWORK_ADDRESS_LENGTH_IP)
   {
-   IPOIB_PRINT(TRACE_LEVEL_INFORMATION, IPOIB_DBG_OID,
+   IPOIB_PRINT(TRACE_LEVEL_ERROR, IPOIB_DBG_ERROR,
     ("Port %d OID_GEN_NETWORK_LAYER_ADDRESSES - Address %d is wrong size of %d, "
      "should be %d\n", port_num, i, p_net_addr_oid->AddressLength,
      NETWORK_ADDRESS_LENGTH_IP));
-   continue;
+   ASSERT ( p_net_addr_oid->AddressLength == NETWORK_ADDRESS_LENGTH_IPX );
+   break;
+
   }
-
+
+  ASSERT( p_net_addr_oid->AddressType == NDIS_PROTOCOL_ID_TCP_IP );
+
+  p_net_addr_oid = (PNETWORK_ADDRESS)((uint8_t *)p_net_addr_oid +
+       FIELD_OFFSET(NETWORK_ADDRESS, Address) +
+       p_net_addr_oid->AddressLength) ;
+
+
   p_ip_addr = (PNETWORK_ADDRESS_IP)p_net_addr_oid->Address;

   /* Size the vector as needed. */
   if( cl_vector_get_size( &p_adapter->ip_vector ) <= idx )
    cl_vector_set_size( &p_adapter->ip_vector, idx + 1 );

-  p_addr_item = cl_vector_get_ptr( &p_adapter->ip_vector, idx );
+  p_addr_item = (net_address_item_t *) cl_vector_get_ptr( &p_adapter->ip_vector, idx );
   if( !cl_memcmp( &p_ip_addr->in_addr, &p_addr_item->address.as_ulong,
    sizeof(ULONG) ) )
   {
Index: B:/users/xalex/MLNX_WinOF-2_1_2/ulp/ipoib_NDIS6_CM/kernel/ipoib_driver.cpp
===================================================================
--- B:/users/xalex/MLNX_WinOF-2_1_2/ulp/ipoib_NDIS6_CM/kernel/ipoib_driver.cpp (revision 6298)
+++ B:/users/xalex/MLNX_WinOF-2_1_2/ulp/ipoib_NDIS6_CM/kernel/ipoib_driver.cpp (revision 6299)
@@ -3514,30 +3514,27 @@
    cl_vector_get_ptr( &p_adapter->ip_vector, idx );
   p_net_addr_oid = (PNETWORK_ADDRESS)p_net_addrs->Address;

-  for( i = 0; i < p_net_addrs->AddressCount; ++i, p_net_addr_oid =
-   (PNETWORK_ADDRESS)((uint8_t *)p_net_addr_oid +
-   FIELD_OFFSET(NETWORK_ADDRESS, Address) +
-   p_net_addr_oid->AddressLength) )
+  for( i = 0; i < p_net_addrs->AddressCount; ++i )
   {

-   if( p_net_addr_oid->AddressType != NDIS_PROTOCOL_ID_TCP_IP )
-   {
-    IPOIB_PRINT( TRACE_LEVEL_WARNING, IPOIB_DBG_OID,
-     ("Port %d OID_GEN_NETWORK_LAYER_ADDRESSES - Address %d is wrong type of 0x%.4X, "
-      "should be 0x%.4X\n", port_num, i, p_net_addr_oid->AddressType,
-      NDIS_PROTOCOL_ID_TCP_IP));
-    continue;
-   }
-
+   // Here we check that the data stored at 'AddressLength' field is valid;
+   // otherwise, it can lead to a memory violation (happened when AddressCount was > 1)
    if( p_net_addr_oid->AddressLength != NETWORK_ADDRESS_LENGTH_IP)
    {
-    IPOIB_PRINT( TRACE_LEVEL_WARNING, IPOIB_DBG_OID,
+    IPOIB_PRINT(TRACE_LEVEL_ERROR, IPOIB_DBG_ERROR,
      ("Port %d OID_GEN_NETWORK_LAYER_ADDRESSES - Address %d is wrong size of %d, "
       "should be %d\n", port_num, i, p_net_addr_oid->AddressLength,
       NETWORK_ADDRESS_LENGTH_IP));
-    continue;
+    ASSERT ( p_net_addr_oid->AddressLength == NETWORK_ADDRESS_LENGTH_IPX );
+    break;
    }
+
+   p_net_addr_oid = (PNETWORK_ADDRESS)((uint8_t *)p_net_addr_oid +
+        FIELD_OFFSET(NETWORK_ADDRESS, Address) +
+        p_net_addr_oid->AddressLength) ;

+   ASSERT( p_net_addr_oid->AddressType == NDIS_PROTOCOL_ID_TCP_IP );
+
    p_ip_addr = (PNETWORK_ADDRESS_IP)p_net_addr_oid->Address;
    if( !cl_memcmp( &p_ip_addr->in_addr,
     &p_addr_item->address.as_ulong, sizeof(ULONG) ) )
@@ -3577,29 +3574,30 @@
  /* Now look for new addresses */
  p_net_addr_oid = (NETWORK_ADDRESS *)p_net_addrs->Address;
  idx = 0;
- for( i = 0; i < p_net_addrs->AddressCount; i++, p_net_addr_oid =
-  (PNETWORK_ADDRESS)((uint8_t *)p_net_addr_oid +
-  FIELD_OFFSET(NETWORK_ADDRESS, Address) + p_net_addr_oid->AddressLength) )
+
+ for( i = 0; i < p_net_addrs->AddressCount; ++i )
  {

-  if( p_net_addr_oid->AddressType != NDIS_PROTOCOL_ID_TCP_IP )
-  {
-   IPOIB_PRINT(TRACE_LEVEL_INFORMATION, IPOIB_DBG_OID,
-    ("Port %d OID_GEN_NETWORK_LAYER_ADDRESSES - Address %d is wrong type of 0x%.4X, "
-     "should be 0x%.4X\n", port_num, i, p_net_addr_oid->AddressType,
-     NDIS_PROTOCOL_ID_TCP_IP));
-   continue;
-  }
-
+  // Here we check that the data stored at 'AddressLength' field is valid;
+  // otherwise, it can lead to a memory violation (happened when AddressCount was > 1)
   if( p_net_addr_oid->AddressLength != NETWORK_ADDRESS_LENGTH_IP)
   {
-   IPOIB_PRINT(TRACE_LEVEL_INFORMATION, IPOIB_DBG_OID,
+   IPOIB_PRINT(TRACE_LEVEL_ERROR, IPOIB_DBG_ERROR,
     ("Port %d OID_GEN_NETWORK_LAYER_ADDRESSES - Address %d is wrong size of %d, "
      "should be %d\n", port_num, i, p_net_addr_oid->AddressLength,
      NETWORK_ADDRESS_LENGTH_IP));
-   continue;
+   ASSERT ( p_net_addr_oid->AddressLength == NETWORK_ADDRESS_LENGTH_IPX );
+   break;
+
   }
-
+
+  ASSERT( p_net_addr_oid->AddressType == NDIS_PROTOCOL_ID_TCP_IP );
+
+  p_net_addr_oid = (PNETWORK_ADDRESS)((uint8_t *)p_net_addr_oid +
+       FIELD_OFFSET(NETWORK_ADDRESS, Address) +
+       p_net_addr_oid->AddressLength) ;
+
+
   p_ip_addr = (PNETWORK_ADDRESS_IP)p_net_addr_oid->Address;

   /* Size the vector as needed. */
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/ofw/attachments/20100823/14837030/attachment.html>