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

Smith, Stan stan.smith at intel.com
Mon Aug 23 14:40:04 PDT 2010


Hello,
  I think Sean is correct in that this sounds like an NDIS problem. That said, NDIS is not going to be fixed soon, so I would agree with your patch for now.
Currently incorporating your patch, will let you know if I can trigger the bug? How is it you trigger the bug which the patch fixes?

thanks,

stan.

PS: anybody know how to convince outlook not to add <tab> chars in the subject line after x characters of subj line?

________________________________
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/5983dda7/attachment.html>


More information about the ofw mailing list