All items in SYSTEM_PROCESSOR array may not be initialized

The SYSTEM_PROCESSOR array is allocated to fit the maximum number of supported
CPUs, but elements are only initialized when the SystemProcessorChangeCallback
callback is called with KeProcessorAddCompleteNotify.
Check if the SYSTEM_PROCESSOR structure is initialized before accessing any
other members, and fail SystemProcessorVcpuId with STATUS_NOT_SUPPORTED for any
uninitialized CPUs

Signed-off-by: Owen Smith <owen.smith@citrix.com>

diff --git a/src/xen/system.c b/src/xen/system.c
index f85efc4fba701cd5164f7f838f3ad78fee82d20f..7220faafd25f8c77d7b5351b8d417b57eb2132eb 100644 (file)
--- a/src/xen/system.c
+++ b/src/xen/system.c
@@ -54,6 +54,7 @@ typedef struct _SYSTEM_PROCESSOR {
     CHAR        Manufacturer[13];
     UCHAR       ApicID;
     UCHAR       ProcessorID;
+    BOOLEAN     Initialized;
     NTSTATUS    Status;
     KEVENT      Event;
     vcpu_info_t *Vcpu;
@@ -643,9 +644,14 @@ SystemProcessorVcpuId(
     if (Cpu >= Context->ProcessorCount)
         goto fail1;
 
+    status = STATUS_NOT_SUPPORTED;
+    if (!Processor->Initialized)
+        goto fail2;
+
     *vcpu_id = Processor->ProcessorID;
     return STATUS_SUCCESS;
 
+fail2:
 fail1:
     return status;
 }
@@ -666,14 +672,18 @@ SystemProcessorVcpuInfo(
         goto fail1;
 
     status = STATUS_NOT_SUPPORTED;
-    if (Processor->Registered == NULL)
+    if (!Processor->Initialized)
         goto fail2;
 
+    if (Processor->Registered == NULL)
+        goto fail3;
+
     ASSERT(*Processor->Registered);
     *Vcpu = Processor->Vcpu;
 
     return STATUS_SUCCESS;
 
+fail3:
 fail2:
 fail1:
     return status;
@@ -702,6 +712,8 @@ SystemProcessorRegisterVcpuInfo(
     if (Cpu >= Context->ProcessorCount)
         goto fail1;
 
+    ASSERT(Processor->Initialized);
+
     ASSERT(Mdl->MdlFlags & MDL_MAPPED_TO_SYSTEM_VA);
     MdlMappedSystemVa = Mdl->MappedSystemVa;
 
@@ -864,6 +876,7 @@ SystemProcessorChangeCallback(
         Processor = &Context->Processor[Cpu];
 
         KeInitializeEvent(&Processor->Event, NotificationEvent, FALSE);
+        Processor->Initialized = TRUE;
 
         KeInitializeDpc(&Processor->Dpc, SystemProcessorDpc, NULL);
         KeSetImportanceDpc(&Processor->Dpc, HighImportance);
@@ -991,9 +1004,12 @@ SystemDeregisterProcessorChangeCallback(
     for (Cpu = 0; Cpu < Context->ProcessorCount; Cpu++) {
         PSYSTEM_PROCESSOR   Processor = &Context->Processor[Cpu];
 
+        // Should check Processor->Initialized, but these operations are harmless to
+        // uninitialized SYSTEM_PROCESSOR structures.
         SystemProcessorDeregisterVcpuInfo(Cpu);
         SystemProcessorTeardown(Cpu);
 
+        Processor->Initialized = FALSE;
         RtlZeroMemory(&Processor->Dpc, sizeof (KDPC));
         RtlZeroMemory(&Processor->Event, sizeof (KEVENT));
         Processor->Status = 0;
@@ -1215,6 +1231,9 @@ SystemCheckProcessors(
     {
         PSYSTEM_PROCESSOR   Processor = &Context->Processor[Cpu];
 
+        if (!Processor->Initialized)
+            continue;
+
         (VOID) KeWaitForSingleObject(&Processor->Event,
                                      Executive,
                                      KernelMode,

diff --git a/src/xenbus/evtchn.c b/src/xenbus/evtchn.c
index 8942cdf30f812e3ac31a065b02c3c5ccc6285543..ccc8f2e06a0653332156c6f458c890ead8604227 100644 (file)
--- a/src/xenbus/evtchn.c
+++ b/src/xenbus/evtchn.c
@@ -284,11 +284,12 @@ EvtchnOpenVirq(
         goto fail1;
 
     status = SystemProcessorVcpuId(Cpu, &vcpu_id);
-    ASSERT(NT_SUCCESS(status));
+    if (!NT_SUCCESS(status))
+        goto fail2;
 
     status = EventChannelBindVirq(Index, vcpu_id, &LocalPort);
     if (!NT_SUCCESS(status))
-        goto fail2;
+        goto fail3;
 
     Channel->Parameters.Virq.Index = Index;
 
@@ -296,6 +297,9 @@ EvtchnOpenVirq(
 
     return STATUS_SUCCESS;
 
+fail3:
+    Error("fail3\n");
+
 fail2:
     Error("fail2\n");
 
@@ -769,11 +773,12 @@ EvtchnBind(
     LocalPort = Channel->LocalPort;
 
     status = SystemProcessorVcpuId(Cpu, &vcpu_id);
-    ASSERT(NT_SUCCESS(status));
+    if (!NT_SUCCESS(status))
+        goto fail2;
 
     status = EventChannelBindVirtualCpu(LocalPort, vcpu_id);
     if (!NT_SUCCESS(status))
-        goto fail2;
+        goto fail3;
 
     Channel->Cpu = Cpu;
 
@@ -784,6 +789,9 @@ done:
 
     return STATUS_SUCCESS;
 
+fail3:
+    Error("fail3\n");
+
 fail2:
     Error("fail2\n");
 
@@ -1292,7 +1300,8 @@ EvtchnInterruptEnable(
             continue;
 
         status = SystemProcessorVcpuId(Cpu, &vcpu_id);
-        ASSERT(NT_SUCCESS(status));
+        if (!NT_SUCCESS(status))
+            continue;
 
         Vector = FdoGetInterruptVector(Context->Fdo, Processor->Interrupt);
 
@@ -1359,7 +1368,8 @@ EvtchnInterruptDisable(
             continue;
 
         status = SystemProcessorVcpuId(Cpu, &vcpu_id);
-        ASSERT(NT_SUCCESS(status));
+        if (!NT_SUCCESS(status))
+            continue;
 
         (VOID) HvmSetEvtchnUpcallVector(vcpu_id, 0);
         Processor->UpcallEnabled = FALSE;

diff --git a/src/xenbus/evtchn_fifo.c b/src/xenbus/evtchn_fifo.c
index 3b3f4938ed73c5a4db239483352d18be99b6434c..a8dab8ca66845bc1e9675fb0bef7e523f71530e3 100644 (file)
--- a/src/xenbus/evtchn_fifo.c
+++ b/src/xenbus/evtchn_fifo.c
@@ -514,13 +514,14 @@ EvtchnFifoAcquire(
             goto fail1;
 
         status = SystemProcessorVcpuId(Index, &vcpu_id);
-        ASSERT(NT_SUCCESS(status));
+        if (!NT_SUCCESS(status))
+            goto fail2;
 
         Pfn = MmGetMdlPfnArray(Mdl)[0];
 
         status = EventChannelInitControl(Pfn, vcpu_id);
         if (!NT_SUCCESS(status))
-            goto fail2;
+            goto fail3;
 
         Address.QuadPart = (ULONGLONG)Pfn << PAGE_SHIFT;
 
@@ -542,6 +543,9 @@ done:
 
     return STATUS_SUCCESS;
 
+fail3:
+    Error("fail3\n");
+
 fail2:
     Error("fail2\n");
 

diff --git a/src/xenbus/shared_info.c b/src/xenbus/shared_info.c
index 9f119791a03a219407731b271d9386b9bbb6f4d1..786ffcf9058ddf5d76da5a7038d457a37670e31e 100644 (file)
--- a/src/xenbus/shared_info.c
+++ b/src/xenbus/shared_info.c
@@ -639,6 +639,8 @@ SharedInfoAcquire(
         Processor = &Context->Processor[Index];
 
         status = SystemProcessorVcpuId(Index, &Processor->vcpu_id);
+        if (status == STATUS_NOT_SUPPORTED)
+            continue;
         if (!NT_SUCCESS(status))
             goto fail7;