[viostor]: Fix a potential memory corruption caused by writing request status

When passing SRB to the host, such as read or write request, the buffers to be read/written are passed in form of so-called scatter-gather lists (SG lists), containing physical addresses and lengths of the buffers. Initial SG lista are passed to viostor (and vioscsi as well) by Storport as part of the SRB. Viostor/vioscsi enlarges each SG list by two small buffers:
- one to specify the command for the host,
- one to receive the result.

However, if a SRB is completed as part of a bus reset request to the driver, the memory to rceive SRB's result no longer belongs to the SRB (since the SRB was completed). The host, however, does not know that, thus, when it completes its work, it writes the result to a memory location no longer owned by the driver.

This commit is a remedy for such a memory corruption. Memory for receiving results for SRB commands is now part of the adapter extension. It seems, that memory allocations during request processing (i.e. BuildIo callback) are not possible for miniports servicing boot disks, thus, the space reserved for receiving SRB results is limited. This means, there is an upper limit on number of SRBs being processed by the driver at once.

Signed-Off-By: Martin Drab <[email protected]>

Author: Martin Drab

viostor/viostor.vcxproj

@@ -221,6 +221,7 @@
     <Inf Include="viostor.inx" />
   </ItemGroup>
   <ItemGroup>
+    <ClInclude Include="virtio_status_table.h" />
     <ClInclude Include="virtio_stor.h" />
     <ClInclude Include="virtio_stor_hw_helper.h" />
     <ClInclude Include="virtio_stor_trace.h" />
@@ -231,6 +232,7 @@
   </ItemGroup>
   <ItemGroup>
     <ClCompile Include="virtio_pci.c" />
+    <ClCompile Include="virtio_status_table.c" />
     <ClCompile Include="virtio_stor.c" />
     <ClCompile Include="virtio_stor_hw_helper.c" />
     <ClCompile Include="virtio_stor_utils.c" />
@@ -239,4 +241,4 @@
   <Import Project="$(MSBuildProjectDirectory)\..\build\Driver.Common.targets" />
   <ImportGroup Label="ExtensionTargets">
   </ImportGroup>
-</Project>
+</Project>

viostor/viostor.vcxproj.filters

@@ -36,6 +36,9 @@
     <ClInclude Include="virtio_stor_trace.h">
       <Filter>Header Files</Filter>
     </ClInclude>
+    <ClInclude Include="virtio_status_table.h">
+      <Filter>Header Files</Filter>
+    </ClInclude>
   </ItemGroup>
   <ItemGroup>
     <ResourceCompile Include="virtio_stor.rc">
@@ -55,5 +58,8 @@
     <ClCompile Include="virtio_stor_utils.c">
       <Filter>Source Files</Filter>
     </ClCompile>
+    <ClCompile Include="virtio_status_table.c">
+      <Filter>Source Files</Filter>
+    </ClCompile>
   </ItemGroup>
-</Project>
+</Project>

viostor/virtio_status_table.c

@@ -0,0 +1,158 @@
+/*
+ * This file contains a hash table for storing SRB statuses received from the host
+ *
+ * Copyright (c) 2025 Virtuozzo
+ *
+ * Author(s):
+ *  Martin Drab <[email protected]>
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met :
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and / or other materials provided with the distribution.
+ * 3. Neither the names of the copyright holders nor the names of their contributors
+ *    may be used to endorse or promote products derived from this software
+ *    without specific prior written permission.
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED.IN NO EVENT SHALL THE COPYRIGHT HOLDERS OR CONTRIBUTORS BE LIABLE
+ * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
+ * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+ * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
+ * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+#include <ntddk.h>
+#include "virtio_stor_trace.h"
+#include "virtio_status_table.h"
+#if defined(EVENT_TRACING)
+#include "virtio_status_table.tmh"
+#endif
+
+static size_t _HashFunctionFirst(ULONG64 Id)
+{
+    size_t ret = 0;
+
+    ret = (size_t)(3 * (Id & 0xff) + 5 * ((Id >> 8) & 0xff) + 7 * ((Id >> 16) & 0xff) + 11 * ((Id >> 24) & 0xff) +
+                   13 * ((Id >> 32) & 0xff) + 17 * ((Id >> 40) & 0xff) + 19 * ((Id >> 48) & 0xff) +
+                   23 * ((Id >> 56) & 0xff));
+
+    return ret;
+}
+
+static size_t _HashFunctionNext(ULONG64 Id, size_t BaseIndex, size_t Attempt)
+{
+    return (BaseIndex + Attempt * (Id & 0xff));
+}
+
+void StatusTableInit(PSTATUS_TABLE Table)
+{
+    memset(Table->Entries, 0, sizeof(Table->Entries));
+    for (size_t i = 0; i < VIRTIO_STATUS_TABLE_SIZE; ++i)
+    {
+        Table->Entries[i].Status = 0xEE;
+    }
+
+    InterlockedExchange(&Table->Lock, 0);
+
+    return;
+}
+
+unsigned char *StatusTableInsert(PSTATUS_TABLE Table, ULONG64 Id)
+{
+    size_t index = 0;
+    size_t attempt = 0;
+    size_t nextIndex = 0;
+    unsigned char *ret = NULL;
+    PSTATUS_TABLE_ENTRY entry = NULL;
+
+    while (InterlockedCompareExchange(&Table->Lock, 1, 0))
+    {
+        __nop();
+    }
+
+    index = _HashFunctionFirst(Id) % VIRTIO_STATUS_TABLE_SIZE;
+    nextIndex = index;
+    do
+    {
+        entry = Table->Entries + nextIndex;
+        if (!entry->Present || entry->Deleted)
+        {
+            entry->Present = 1;
+            entry->Deleted = 0;
+            entry->Id = Id;
+            ret = &entry->Status;
+            break;
+        }
+
+        ++attempt;
+        nextIndex = _HashFunctionNext(Id, index, attempt) % VIRTIO_STATUS_TABLE_SIZE;
+    } while (index != nextIndex);
+
+    InterlockedExchange(&Table->Lock, 0);
+    if (index == nextIndex && attempt > 0)
+    {
+        RhelDbgPrint(TRACE_LEVEL_WARNING, " Unable to insert status for ID %llu\n", Id);
+    }
+
+    return ret;
+}
+
+void StatusTableDelete(PSTATUS_TABLE Table, ULONG64 Id, unsigned char *Status)
+{
+    size_t index = 0;
+    size_t attempt = 0;
+    size_t nextIndex = 0;
+    unsigned char *ret = NULL;
+    PSTATUS_TABLE_ENTRY entry = NULL;
+
+    while (InterlockedCompareExchange(&Table->Lock, 1, 0))
+    {
+        __nop();
+    }
+
+    index = _HashFunctionFirst(Id) % VIRTIO_STATUS_TABLE_SIZE;
+    nextIndex = index;
+    do
+    {
+        entry = Table->Entries + nextIndex;
+        if (!entry->Present && !entry->Deleted)
+        {
+            nextIndex = index;
+            attempt = 1;
+            break;
+        }
+
+        if (entry->Present && entry->Id == Id)
+        {
+            entry->Deleted = 1;
+            entry->Present = 0;
+            entry->Id = 0;
+            if (Status != NULL)
+            {
+                *Status = entry->Status;
+            }
+
+            entry->Status = 0xEE;
+            break;
+        }
+
+        ++attempt;
+        nextIndex = _HashFunctionNext(Id, index, attempt) % VIRTIO_STATUS_TABLE_SIZE;
+    } while (index != nextIndex);
+
+    InterlockedExchange(&Table->Lock, 0);
+    if (index == nextIndex && attempt > 0)
+    {
+        RhelDbgPrint(TRACE_LEVEL_ERROR, " Unable to delete status for ID %llu\n", Id);
+    }
+
+    return;
+}

viostor/virtio_status_table.h

@@ -0,0 +1,55 @@
+/*
+ * Include file for the SRB status table
+ * 
+ * Copyright (c) 2025 Virtuozzo
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met :
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and / or other materials provided with the distribution.
+ * 3. Neither the names of the copyright holders nor the names of their contributors
+ *    may be used to endorse or promote products derived from this software
+ *    without specific prior written permission.
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED.IN NO EVENT SHALL THE COPYRIGHT HOLDERS OR CONTRIBUTORS BE LIABLE
+ * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
+ * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+ * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
+ * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#ifndef __VIRTIO_STATUS_TABLE_H__
+#define __VIRTIO_STATUS_TABLE_H__
+
+#include <ntddk.h>
+
+typedef struct _STATUS_TABLE_ENTRY
+{
+    unsigned char Status;
+    ULONG64 Id;
+    int Present : 1;
+    int Deleted : 1;
+} STATUS_TABLE_ENTRY, *PSTATUS_TABLE_ENTRY;
+
+#define VIRTIO_STATUS_TABLE_SIZE 769
+
+typedef struct _STATUS_TABLE
+{
+    STATUS_TABLE_ENTRY Entries[VIRTIO_STATUS_TABLE_SIZE];
+    volatile LONG Lock;
+} STATUS_TABLE, *PSTATUS_TABLE;
+
+void StatusTableInit(PSTATUS_TABLE Table);
+unsigned char *StatusTableInsert(PSTATUS_TABLE Table, ULONG64 Id);
+void StatusTableDelete(PSTATUS_TABLE Table, ULONG64 Id, unsigned char *Status);
+
+#endif

viostor/virtio_stor.c

@@ -259,6 +259,7 @@ VirtIoFindAdapter(IN PVOID DeviceExtension,
 
     adaptExt = (PADAPTER_EXTENSION)DeviceExtension;
 
+    StatusTableInit(&adaptExt->StatusTable);
     adaptExt->last_srb_id = 1;
     adaptExt->system_io_bus_number = ConfigInfo->SystemIoBusNumber;
     adaptExt->slot_number = ConfigInfo->SlotNumber;
@@ -2124,9 +2125,11 @@ VOID VioStorCompleteRequest(IN PVOID DeviceExtension, IN ULONG MessageID, IN BOO
         {
             PLIST_ENTRY le = NULL;
             BOOLEAN bFound = FALSE;
+            unsigned char vbrStatus = 0;
 #ifdef DBG
             InterlockedDecrement((LONG volatile *)&adaptExt->inqueue_cnt);
 #endif
+            StatusTableDelete(&adaptExt->StatusTable, srbId, &vbrStatus);
             for (le = element->srb_list.Flink; le != &element->srb_list && !bFound; le = le->Flink)
             {
                 pblk_req req = CONTAINING_RECORD(le, blk_req, list_entry);
@@ -2140,6 +2143,7 @@ VOID VioStorCompleteRequest(IN PVOID DeviceExtension, IN ULONG MessageID, IN BOO
                 _Analysis_assume_(srbExt != NULL);
                 if (srbExt->id == srbId)
                 {
+                    srbExt->vbr.status = vbrStatus;
                     RemoveEntryList(le);
                     element->srb_cnt--;
                     bFound = TRUE;

viostor/virtio_stor.h

@@ -41,6 +41,7 @@
 #include "virtio_ring.h"
 #include "virtio_stor_utils.h"
 #include "virtio_stor_hw_helper.h"
+#include "virtio_status_table.h"
 
 typedef struct VirtIOBufferDescriptor VIO_SG, *PVIO_SG;
 
@@ -257,6 +258,7 @@ typedef struct _ADAPTER_EXTENSION
     BOOLEAN reset_in_progress;
     ULONGLONG fw_ver;
     ULONG_PTR last_srb_id;
+    STATUS_TABLE StatusTable;
 #ifdef DBG
     LONG srb_cnt;
     LONG inqueue_cnt;