Avoid race when checking for an active transaction

The code in StoreTransactionEnd() checks, under the protection of Context->Lock,
that the transaction is active. It then drops the lock and calls
StorePrepareRequest(), which also tests whether the transaction is active but
*not* under the protection of the lock. If the domain is suspended and resumed
in between the two checks then this will cause StorePrepareRequest() to fail
for non-NULL transactions.
This patch makes sure that Context->Lock is held across all calls to
StorePrepareRequest(), along with any prior tests for whether transactions or
watches are active, and drops the internal acquisition of the lock (which was
done to protect the increment of Context->RequestId).

Reported-by: Owen Smith <owen.smith@citrix.com>
Signed-off-by: Paul Durrant <pdurrant@amazon.com>

diff --git a/src/xenbus/store.c b/src/xenbus/store.c
index 5ffea1fe48595b77bf44fb67a53a1aad410e4ff3..028fd59f07b6af0b8fe8da8ac8b596436509eebc 100644 (file)
--- a/src/xenbus/store.c
+++ b/src/xenbus/store.c
@@ -180,7 +180,6 @@ StorePrepareRequest(
     )
 {
     ULONG                           Id;
-    KIRQL                           Irql;
     PXENBUS_STORE_SEGMENT           Segment;
     va_list                         Arguments;
     NTSTATUS                        status;
@@ -200,10 +199,7 @@ StorePrepareRequest(
     Request->Header.type = Type;
     Request->Header.tx_id = Id;
     Request->Header.len = 0;
-
-    KeAcquireSpinLock(&Context->Lock, &Irql);
     Request->Header.req_id = Context->RequestId++;
-    KeReleaseSpinLock(&Context->Lock, Irql);
 
     Request->Count = 0;
     Segment = &Request->Segment[Request->Count++];
@@ -1102,6 +1098,7 @@ StoreRead(
     PXENBUS_STORE_CONTEXT           Context = Interface->Context;
     PVOID                           Caller;
     XENBUS_STORE_REQUEST            Request;
+    KIRQL                           Irql;
     PXENBUS_STORE_RESPONSE          Response;
     PXENBUS_STORE_BUFFER            Buffer;
     NTSTATUS                        status;
@@ -1110,6 +1107,8 @@ StoreRead(
 
     RtlZeroMemory(&Request, sizeof (XENBUS_STORE_REQUEST));
 
+    KeAcquireSpinLock(&Context->Lock, &Irql);
+
     if (Prefix == NULL) {
         status = StorePrepareRequest(Context,
                                      &Request,
@@ -1130,6 +1129,8 @@ StoreRead(
                                      NULL, 0);
     }
 
+    KeReleaseSpinLock(&Context->Lock, Irql);
+
     if (!NT_SUCCESS(status))
         goto fail1;
 
@@ -1177,11 +1178,14 @@ StoreWrite(
     )
 {
     XENBUS_STORE_REQUEST            Request;
+    KIRQL                           Irql;
     PXENBUS_STORE_RESPONSE          Response;
     NTSTATUS                        status;
 
     RtlZeroMemory(&Request, sizeof (XENBUS_STORE_REQUEST));
 
+    KeAcquireSpinLock(&Context->Lock, &Irql);
+
     if (Prefix == NULL) {
         status = StorePrepareRequest(Context,
                                      &Request,
@@ -1204,6 +1208,8 @@ StoreWrite(
                                      NULL, 0);
     }
 
+    KeReleaseSpinLock(&Context->Lock, Irql);
+
     if (!NT_SUCCESS(status))
         goto fail1;
 
@@ -1326,11 +1332,14 @@ StoreRemove(
 {
     PXENBUS_STORE_CONTEXT           Context = Interface->Context;
     XENBUS_STORE_REQUEST            Request;
+    KIRQL                           Irql;
     PXENBUS_STORE_RESPONSE          Response;
     NTSTATUS                        status;
 
     RtlZeroMemory(&Request, sizeof (XENBUS_STORE_REQUEST));
 
+    KeAcquireSpinLock(&Context->Lock, &Irql);
+
     if (Prefix == NULL) {
         status = StorePrepareRequest(Context,
                                      &Request,
@@ -1351,6 +1360,8 @@ StoreRemove(
                                      NULL, 0);
     }
 
+    KeReleaseSpinLock(&Context->Lock, Irql);
+
     if (!NT_SUCCESS(status))
         goto fail1;
 
@@ -1391,6 +1402,7 @@ StoreDirectory(
     PXENBUS_STORE_CONTEXT           Context = Interface->Context;
     PVOID                           Caller;
     XENBUS_STORE_REQUEST            Request;
+    KIRQL                           Irql;
     PXENBUS_STORE_RESPONSE          Response;
     PXENBUS_STORE_BUFFER            Buffer;
     NTSTATUS                        status;
@@ -1399,6 +1411,8 @@ StoreDirectory(
 
     RtlZeroMemory(&Request, sizeof (XENBUS_STORE_REQUEST));
 
+    KeAcquireSpinLock(&Context->Lock, &Irql);
+
     if (Prefix == NULL) {
         status = StorePrepareRequest(Context,
                                      &Request,
@@ -1419,6 +1433,8 @@ StoreDirectory(
                                      NULL, 0);
     }
 
+    KeReleaseSpinLock(&Context->Lock, Irql);
+
     if (!NT_SUCCESS(status))
         goto fail1;
 
@@ -1486,12 +1502,17 @@ StoreTransactionStart(
 
     RtlZeroMemory(&Request, sizeof (XENBUS_STORE_REQUEST));
 
+    KeAcquireSpinLock(&Context->Lock, &Irql);
+
     status = StorePrepareRequest(Context,
                                  &Request,
                                  NULL,
                                  XS_TRANSACTION_START,
                                  "", 1,
                                  NULL, 0);
+
+    KeReleaseSpinLock(&Context->Lock, Irql);
+
     ASSERT(NT_SUCCESS(status));
 
     Response = StoreSubmitRequest(Context, &Request);
@@ -1555,22 +1576,23 @@ StoreTransactionEnd(
 
     ASSERT3U(Transaction->Magic, ==, STORE_TRANSACTION_MAGIC);
 
+    RtlZeroMemory(&Request, sizeof (XENBUS_STORE_REQUEST));
+
     KeAcquireSpinLock(&Context->Lock, &Irql);
 
     status = STATUS_RETRY;
     if (!Transaction->Active)
         goto done;
 
-    KeReleaseSpinLock(&Context->Lock, Irql);
-
-    RtlZeroMemory(&Request, sizeof (XENBUS_STORE_REQUEST));
-
     status = StorePrepareRequest(Context,
                                  &Request,
                                  Transaction,
                                  XS_TRANSACTION_END,
                                  (Commit) ? "T" : "F", 2,
                                  NULL, 0);
+
+    KeReleaseSpinLock(&Context->Lock, Irql);
+
     ASSERT(NT_SUCCESS(status));
 
     Response = StoreSubmitRequest(Context, &Request);
@@ -1678,6 +1700,8 @@ StoreWatchAdd(
 
     RtlZeroMemory(&Request, sizeof (XENBUS_STORE_REQUEST));
 
+    KeAcquireSpinLock(&Context->Lock, &Irql);
+
     status = StorePrepareRequest(Context,
                                  &Request,
                                  NULL,
@@ -1687,6 +1711,9 @@ StoreWatchAdd(
                                  Token, strlen(Token), 
                                  "", 1,
                                  NULL, 0);
+
+    KeReleaseSpinLock(&Context->Lock, Irql);
+
     ASSERT(NT_SUCCESS(status));
 
     Response = StoreSubmitRequest(Context, &Request);
@@ -1760,13 +1787,6 @@ StoreWatchRemove(
 
     Path = Watch->Path;
 
-    KeAcquireSpinLock(&Context->Lock, &Irql);
-
-    if (!Watch->Active)
-        goto done;
-
-    KeReleaseSpinLock(&Context->Lock, Irql);
-
     status = RtlStringCbPrintfA(Token,
                                 sizeof (Token),
                                 "TOK|%p|%04X",
@@ -1777,6 +1797,11 @@ StoreWatchRemove(
 
     RtlZeroMemory(&Request, sizeof (XENBUS_STORE_REQUEST));
 
+    KeAcquireSpinLock(&Context->Lock, &Irql);
+
+    if (!Watch->Active)
+        goto done;
+
     status = StorePrepareRequest(Context,
                                  &Request,
                                  NULL,
@@ -1786,6 +1811,9 @@ StoreWatchRemove(
                                  Token, strlen(Token), 
                                  "", 1,
                                  NULL, 0);
+
+    KeReleaseSpinLock(&Context->Lock, Irql);
+
     ASSERT(NT_SUCCESS(status));
 
     Response = StoreSubmitRequest(Context, &Request);
@@ -2002,6 +2030,7 @@ StorePermissionsSet(
 {
     PXENBUS_STORE_CONTEXT           Context = Interface->Context;
     XENBUS_STORE_REQUEST            Request;
+    KIRQL                           Irql;
     PXENBUS_STORE_RESPONSE          Response;
     NTSTATUS                        status;
     ULONG                           Index;
@@ -2048,6 +2077,8 @@ StorePermissionsSet(
         Length -= Used;
     }
 
+    KeAcquireSpinLock(&Context->Lock, &Irql);
+
     status = StorePrepareRequest(Context,
                                  &Request,
                                  Transaction,
@@ -2056,6 +2087,9 @@ StorePermissionsSet(
                                  "", 1,
                                  PermissionString, XENSTORE_PAYLOAD_MAX - Length,
                                  NULL, 0);
+
+    KeReleaseSpinLock(&Context->Lock, Irql);
+
     if (!NT_SUCCESS(status))
         goto fail4;