Skip to content

Commit a8d0d42

Browse files
author
Martin Drab
committed
[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 <martin.drab@virtuozzo.com>
1 parent 29fc692 commit a8d0d42

File tree

7 files changed

+319
-160
lines changed

7 files changed

+319
-160
lines changed

viostor/viostor.vcxproj

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -221,6 +221,7 @@
221221
<Inf Include="viostor.inx" />
222222
</ItemGroup>
223223
<ItemGroup>
224+
<ClInclude Include="virtio_status_table.h" />
224225
<ClInclude Include="virtio_stor.h" />
225226
<ClInclude Include="virtio_stor_hw_helper.h" />
226227
<ClInclude Include="virtio_stor_trace.h" />
@@ -231,6 +232,7 @@
231232
</ItemGroup>
232233
<ItemGroup>
233234
<ClCompile Include="virtio_pci.c" />
235+
<ClCompile Include="virtio_status_table.c" />
234236
<ClCompile Include="virtio_stor.c" />
235237
<ClCompile Include="virtio_stor_hw_helper.c" />
236238
<ClCompile Include="virtio_stor_utils.c" />
@@ -239,4 +241,4 @@
239241
<Import Project="$(MSBuildProjectDirectory)\..\build\Driver.Common.targets" />
240242
<ImportGroup Label="ExtensionTargets">
241243
</ImportGroup>
242-
</Project>
244+
</Project>

viostor/viostor.vcxproj.filters

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,9 @@
3636
<ClInclude Include="virtio_stor_trace.h">
3737
<Filter>Header Files</Filter>
3838
</ClInclude>
39+
<ClInclude Include="virtio_status_table.h">
40+
<Filter>Header Files</Filter>
41+
</ClInclude>
3942
</ItemGroup>
4043
<ItemGroup>
4144
<ResourceCompile Include="virtio_stor.rc">
@@ -55,5 +58,8 @@
5558
<ClCompile Include="virtio_stor_utils.c">
5659
<Filter>Source Files</Filter>
5760
</ClCompile>
61+
<ClCompile Include="virtio_status_table.c">
62+
<Filter>Source Files</Filter>
63+
</ClCompile>
5864
</ItemGroup>
59-
</Project>
65+
</Project>

viostor/virtio_status_table.c

Lines changed: 158 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,158 @@
1+
/*
2+
* This file contains a hash table for storing SRB statuses received from the host
3+
*
4+
* Copyright (c) 2025 Virtuozzo
5+
*
6+
* Author(s):
7+
* Martin Drab <martin.drab@virtuozzo.com>
8+
*
9+
* Redistribution and use in source and binary forms, with or without
10+
* modification, are permitted provided that the following conditions
11+
* are met :
12+
* 1. Redistributions of source code must retain the above copyright
13+
* notice, this list of conditions and the following disclaimer.
14+
* 2. Redistributions in binary form must reproduce the above copyright
15+
* notice, this list of conditions and the following disclaimer in the
16+
* documentation and / or other materials provided with the distribution.
17+
* 3. Neither the names of the copyright holders nor the names of their contributors
18+
* may be used to endorse or promote products derived from this software
19+
* without specific prior written permission.
20+
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS ``AS IS'' AND
21+
* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
22+
* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
23+
* ARE DISCLAIMED.IN NO EVENT SHALL THE COPYRIGHT HOLDERS OR CONTRIBUTORS BE LIABLE
24+
* FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
25+
* DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
26+
* OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
27+
* HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
28+
* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
29+
* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
30+
* SUCH DAMAGE.
31+
*/
32+
#include <ntddk.h>
33+
#include "virtio_stor_trace.h"
34+
#include "virtio_status_table.h"
35+
#if defined(EVENT_TRACING)
36+
#include "virtio_status_table.tmh"
37+
#endif
38+
39+
static size_t _HashFunctionFirst(ULONG64 Id)
40+
{
41+
size_t ret = 0;
42+
43+
ret = (size_t)(3 * (Id & 0xff) + 5 * ((Id >> 8) & 0xff) + 7 * ((Id >> 16) & 0xff) + 11 * ((Id >> 24) & 0xff) +
44+
13 * ((Id >> 32) & 0xff) + 17 * ((Id >> 40) & 0xff) + 19 * ((Id >> 48) & 0xff) +
45+
23 * ((Id >> 56) & 0xff));
46+
47+
return ret;
48+
}
49+
50+
static size_t _HashFunctionNext(ULONG64 Id, size_t BaseIndex, size_t Attempt)
51+
{
52+
return (BaseIndex + Attempt * (Id & 0xff));
53+
}
54+
55+
void StatusTableInit(PSTATUS_TABLE Table)
56+
{
57+
memset(Table->Entries, 0, sizeof(Table->Entries));
58+
for (size_t i = 0; i < VIRTIO_STATUS_TABLE_SIZE; ++i)
59+
{
60+
Table->Entries[i].Status = 0xEE;
61+
}
62+
63+
InterlockedExchange(&Table->Lock, 0);
64+
65+
return;
66+
}
67+
68+
unsigned char *StatusTableInsert(PSTATUS_TABLE Table, ULONG64 Id)
69+
{
70+
size_t index = 0;
71+
size_t attempt = 0;
72+
size_t nextIndex = 0;
73+
unsigned char *ret = NULL;
74+
PSTATUS_TABLE_ENTRY entry = NULL;
75+
76+
while (InterlockedCompareExchange(&Table->Lock, 1, 0))
77+
{
78+
__nop();
79+
}
80+
81+
index = _HashFunctionFirst(Id) % VIRTIO_STATUS_TABLE_SIZE;
82+
nextIndex = index;
83+
do
84+
{
85+
entry = Table->Entries + nextIndex;
86+
if (!entry->Present || entry->Deleted)
87+
{
88+
entry->Present = 1;
89+
entry->Deleted = 0;
90+
entry->Id = Id;
91+
ret = &entry->Status;
92+
break;
93+
}
94+
95+
++attempt;
96+
nextIndex = _HashFunctionNext(Id, index, attempt) % VIRTIO_STATUS_TABLE_SIZE;
97+
} while (index != nextIndex);
98+
99+
InterlockedExchange(&Table->Lock, 0);
100+
if (index == nextIndex && attempt > 0)
101+
{
102+
RhelDbgPrint(TRACE_LEVEL_WARNING, " Unable to insert status for ID %llu\n", Id);
103+
}
104+
105+
return ret;
106+
}
107+
108+
void StatusTableDelete(PSTATUS_TABLE Table, ULONG64 Id, unsigned char *Status)
109+
{
110+
size_t index = 0;
111+
size_t attempt = 0;
112+
size_t nextIndex = 0;
113+
unsigned char *ret = NULL;
114+
PSTATUS_TABLE_ENTRY entry = NULL;
115+
116+
while (InterlockedCompareExchange(&Table->Lock, 1, 0))
117+
{
118+
__nop();
119+
}
120+
121+
index = _HashFunctionFirst(Id) % VIRTIO_STATUS_TABLE_SIZE;
122+
nextIndex = index;
123+
do
124+
{
125+
entry = Table->Entries + nextIndex;
126+
if (!entry->Present && !entry->Deleted)
127+
{
128+
nextIndex = index;
129+
attempt = 1;
130+
break;
131+
}
132+
133+
if (entry->Present && entry->Id == Id)
134+
{
135+
entry->Deleted = 1;
136+
entry->Present = 0;
137+
entry->Id = 0;
138+
if (Status != NULL)
139+
{
140+
*Status = entry->Status;
141+
}
142+
143+
entry->Status = 0xEE;
144+
break;
145+
}
146+
147+
++attempt;
148+
nextIndex = _HashFunctionNext(Id, index, attempt) % VIRTIO_STATUS_TABLE_SIZE;
149+
} while (index != nextIndex);
150+
151+
InterlockedExchange(&Table->Lock, 0);
152+
if (index == nextIndex && attempt > 0)
153+
{
154+
RhelDbgPrint(TRACE_LEVEL_ERROR, " Unable to delete status for ID %llu\n", Id);
155+
}
156+
157+
return;
158+
}

viostor/virtio_status_table.h

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
/*
2+
* Include file for the SRB status table
3+
*
4+
* Copyright (c) 2025 Virtuozzo
5+
*
6+
* Redistribution and use in source and binary forms, with or without
7+
* modification, are permitted provided that the following conditions
8+
* are met :
9+
* 1. Redistributions of source code must retain the above copyright
10+
* notice, this list of conditions and the following disclaimer.
11+
* 2. Redistributions in binary form must reproduce the above copyright
12+
* notice, this list of conditions and the following disclaimer in the
13+
* documentation and / or other materials provided with the distribution.
14+
* 3. Neither the names of the copyright holders nor the names of their contributors
15+
* may be used to endorse or promote products derived from this software
16+
* without specific prior written permission.
17+
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS ``AS IS'' AND
18+
* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
19+
* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
20+
* ARE DISCLAIMED.IN NO EVENT SHALL THE COPYRIGHT HOLDERS OR CONTRIBUTORS BE LIABLE
21+
* FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
22+
* DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
23+
* OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
24+
* HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
25+
* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
26+
* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
27+
* SUCH DAMAGE.
28+
*/
29+
30+
#ifndef __VIRTIO_STATUS_TABLE_H__
31+
#define __VIRTIO_STATUS_TABLE_H__
32+
33+
#include <ntddk.h>
34+
35+
typedef struct _STATUS_TABLE_ENTRY
36+
{
37+
unsigned char Status;
38+
ULONG64 Id;
39+
int Present : 1;
40+
int Deleted : 1;
41+
} STATUS_TABLE_ENTRY, *PSTATUS_TABLE_ENTRY;
42+
43+
#define VIRTIO_STATUS_TABLE_SIZE 769
44+
45+
typedef struct _STATUS_TABLE
46+
{
47+
STATUS_TABLE_ENTRY Entries[VIRTIO_STATUS_TABLE_SIZE];
48+
volatile LONG Lock;
49+
} STATUS_TABLE, *PSTATUS_TABLE;
50+
51+
void StatusTableInit(PSTATUS_TABLE Table);
52+
unsigned char *StatusTableInsert(PSTATUS_TABLE Table, ULONG64 Id);
53+
void StatusTableDelete(PSTATUS_TABLE Table, ULONG64 Id, unsigned char *Status);
54+
55+
#endif

viostor/virtio_stor.c

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -259,6 +259,7 @@ VirtIoFindAdapter(IN PVOID DeviceExtension,
259259

260260
adaptExt = (PADAPTER_EXTENSION)DeviceExtension;
261261

262+
StatusTableInit(&adaptExt->StatusTable);
262263
adaptExt->last_srb_id = 1;
263264
adaptExt->system_io_bus_number = ConfigInfo->SystemIoBusNumber;
264265
adaptExt->slot_number = ConfigInfo->SlotNumber;
@@ -2124,9 +2125,11 @@ VOID VioStorCompleteRequest(IN PVOID DeviceExtension, IN ULONG MessageID, IN BOO
21242125
{
21252126
PLIST_ENTRY le = NULL;
21262127
BOOLEAN bFound = FALSE;
2128+
unsigned char vbrStatus = 0;
21272129
#ifdef DBG
21282130
InterlockedDecrement((LONG volatile *)&adaptExt->inqueue_cnt);
21292131
#endif
2132+
StatusTableDelete(&adaptExt->StatusTable, srbId, &vbrStatus);
21302133
for (le = element->srb_list.Flink; le != &element->srb_list && !bFound; le = le->Flink)
21312134
{
21322135
pblk_req req = CONTAINING_RECORD(le, blk_req, list_entry);
@@ -2140,6 +2143,7 @@ VOID VioStorCompleteRequest(IN PVOID DeviceExtension, IN ULONG MessageID, IN BOO
21402143
_Analysis_assume_(srbExt != NULL);
21412144
if (srbExt->id == srbId)
21422145
{
2146+
srbExt->vbr.status = vbrStatus;
21432147
RemoveEntryList(le);
21442148
element->srb_cnt--;
21452149
bFound = TRUE;

viostor/virtio_stor.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@
4141
#include "virtio_ring.h"
4242
#include "virtio_stor_utils.h"
4343
#include "virtio_stor_hw_helper.h"
44+
#include "virtio_status_table.h"
4445

4546
typedef struct VirtIOBufferDescriptor VIO_SG, *PVIO_SG;
4647

@@ -257,6 +258,7 @@ typedef struct _ADAPTER_EXTENSION
257258
BOOLEAN reset_in_progress;
258259
ULONGLONG fw_ver;
259260
ULONG_PTR last_srb_id;
261+
STATUS_TABLE StatusTable;
260262
#ifdef DBG
261263
LONG srb_cnt;
262264
LONG inqueue_cnt;

0 commit comments

Comments
 (0)