BalloonReleasePfnArray() is broken in several ways

When BalloonReleasePfnArray() attempts a decrease_reservation operation
then it is possible that not all the requested pages will be released.
In this eventuality, the function is supposed to pull the excess PFNs
that were added to the rangeset back out again and then allow the caller,
BalloonInflate(), to free the PFNs back to Windows.

The first bit of brokenness is that the arguments to RangeSetGet() are
permuted, such that it tries to get a range starting at PFN 1 rather than
a single PFN.

The next bit of brokenness is that the loop zeroes out the PFN value from
the array (presumably to satisfy the subsequent bogus ASSERTion) thus
causing the call to BalloonFreePfnArray() made by BalloonInflate() to
attempt to free PFN 0 potentially multiple times.

This patch fixes the code to do what it was intended to do.

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

diff --git a/src/xenbus/balloon.c b/src/xenbus/balloon.c
index 751dd537e1c8a0e47f79d714d9d8ab6bbab82c8e..6777bdff3da1d90817582bf863dacc8edee80fdb 100644 (file)
--- a/src/xenbus/balloon.c
+++ b/src/xenbus/balloon.c
@@ -489,15 +489,13 @@ BalloonReleasePfnArray(
         status = XENBUS_RANGE_SET(Get,
                                   &Context->RangeSetInterface,
                                   Context->RangeSet,
-                                  1,
-                                  (LONGLONG)Context->PfnArray[Index]);
+                                  (LONGLONG)Context->PfnArray[Index],
+                                  1);
         ASSERT(NT_SUCCESS(status));
-
-        Context->PfnArray[Index] = 0;
     }
 
 done:
-    ASSERT(IsZeroMemory(Context->PfnArray, Requested * sizeof (PFN_NUMBER)));
+    ASSERT(IsZeroMemory(Context->PfnArray, Count * sizeof (PFN_NUMBER)));
 
     KeQuerySystemTime(&End);
     TimeDelta = __max(((End.QuadPart - Start.QuadPart) / 10000ull), 1);