[Xen-devel] [PATCH 07/11] OvmfPkg/XenBusDxe: Use on stack buffer in internal functions

From: Anthony PERARD <anthony.perard@citrix.com>
To: <devel@edk2.groups.io>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	Jordan Justen <jordan.l.justen@intel.com>,
	Julien Grall <julien.grall@arm.com>,
	Anthony Perard <anthony.perard@citrix.com>,
	xen-devel@lists.xenproject.org, Laszlo Ersek <lersek@redhat.com>
Subject: [Xen-devel] [PATCH 07/11] OvmfPkg/XenBusDxe: Use on stack buffer in internal functions
Date: Fri, 13 Sep 2019 15:50:56 +0100	[thread overview]
Message-ID: <20190913145100.303433-8-anthony.perard@citrix.com> (raw)
In-Reply-To: <20190913145100.303433-1-anthony.perard@citrix.com>

We will use a buffer on the stack instead of allocating memory for
internal functions that are expecting a reply from xenstore.

The external interface XENBUS_PROTOCOL isn't changed yet, so
allocation are made for XsRead and XsBackendRead.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2190
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 OvmfPkg/XenBusDxe/XenBus.c   |  40 ++++++------
 OvmfPkg/XenBusDxe/XenStore.c | 115 ++++++++++++++++++++---------------
 OvmfPkg/XenBusDxe/XenStore.h |  17 +++---
 3 files changed, 95 insertions(+), 77 deletions(-)

diff --git a/OvmfPkg/XenBusDxe/XenBus.c b/OvmfPkg/XenBusDxe/XenBus.c
index bb8ddbc4d4..78835ec7b3 100644
--- a/OvmfPkg/XenBusDxe/XenBus.c
+++ b/OvmfPkg/XenBusDxe/XenBus.c
@@ -89,19 +89,18 @@ XenBusReadDriverState (
   IN CONST CHAR8 *Path
   )
 {
-  XenbusState State;
-  CHAR8 *Ptr = NULL;
+  XenbusState     State;
+  CHAR8           Buffer[4];
+  UINTN           BufferSize;
   XENSTORE_STATUS Status;
 
-  Status = XenStoreRead (XST_NIL, Path, "state", NULL, (VOID **)&Ptr);
+  BufferSize = sizeof (Buffer) - 1;
+  Status = XenStoreRead (XST_NIL, Path, "state", &BufferSize, Buffer);
   if (Status != XENSTORE_STATUS_SUCCESS) {
     State = XenbusStateClosed;
   } else {
-    State = AsciiStrDecimalToUintn (Ptr);
-  }
-
-  if (Ptr != NULL) {
-    FreePool (Ptr);
+    Buffer[BufferSize] = '\0';
+    State = AsciiStrDecimalToUintn (Buffer);
   }
 
   return State;
@@ -129,8 +128,11 @@ XenBusAddDevice (
 
   if (XenStorePathExists (XST_NIL, DevicePath, "")) {
     XENBUS_PRIVATE_DATA *Child;
-    enum xenbus_state State;
-    CHAR8 *BackendPath;
+    enum xenbus_state   State;
+    CHAR8               BackendPath[XENSTORE_ABS_PATH_MAX + 1];
+    UINTN               BackendPathSize;
+
+    BackendPathSize = sizeof (BackendPath);
 
     Child = XenBusDeviceInitialized (Dev, DevicePath);
     if (Child != NULL) {
@@ -155,17 +157,18 @@ XenBusAddDevice (
     }
 
     StatusXenStore = XenStoreRead (XST_NIL, DevicePath, "backend",
-                                   NULL, (VOID **) &BackendPath);
+      &BackendPathSize, BackendPath);
     if (StatusXenStore != XENSTORE_STATUS_SUCCESS) {
       DEBUG ((EFI_D_ERROR, "xenbus: %a no backend path.\n", DevicePath));
       Status = EFI_NOT_FOUND;
       goto out;
     }
+    BackendPath[BackendPathSize] = '\0';
 
     Private = AllocateCopyPool (sizeof (*Private), &gXenBusPrivateData);
     Private->XenBusIo.Type = AsciiStrDup (Type);
     Private->XenBusIo.Node = AsciiStrDup (DevicePath);
-    Private->XenBusIo.Backend = BackendPath;
+    Private->XenBusIo.Backend = AsciiStrDup (BackendPath);
     Private->XenBusIo.DeviceId = (UINT16)AsciiStrDecimalToUintn (Id);
     Private->Dev = Dev;
 
@@ -309,17 +312,20 @@ XenBusSetState (
   )
 {
   enum xenbus_state CurrentState;
-  XENSTORE_STATUS Status;
-  CHAR8 *Temp;
+  XENSTORE_STATUS   Status;
+  CHAR8             Buffer[4];
+  UINTN             BufferSize;
+
+  BufferSize = sizeof (Buffer) - 1;
 
   DEBUG ((EFI_D_INFO, "XenBus: Set state to %d\n", NewState));
 
-  Status = XenStoreRead (Transaction, This->Node, "state", NULL, (VOID **)&Temp);
+  Status = XenStoreRead (Transaction, This->Node, "state", &BufferSize, Buffer);
   if (Status != XENSTORE_STATUS_SUCCESS) {
     goto Out;
   }
-  CurrentState = AsciiStrDecimalToUintn (Temp);
-  FreePool (Temp);
+  Buffer[BufferSize] = '\0';
+  CurrentState = AsciiStrDecimalToUintn (Buffer);
   if (CurrentState == NewState) {
     goto Out;
   }
diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
index 004d3b6022..b9588bb8c6 100644
--- a/OvmfPkg/XenBusDxe/XenStore.c
+++ b/OvmfPkg/XenBusDxe/XenStore.c
@@ -756,8 +756,9 @@ XenStoreGetError (
   @param RequestType    The type of message to send.
   @param WriteRequest   Pointers to the body sections of the request.
   @param NumRequests    The number of body sections in the request.
-  @param LenPtr         The returned length of the reply.
-  @param ResultPtr      The returned body of the reply.
+  @param BufferSize     IN: size of the buffer
+                        OUT: The returned length of the reply.
+  @param Buffer         The returned body of the reply.
 
   @return  XENSTORE_STATUS_SUCCESS on success.  Otherwise an errno indicating
            the cause of failure.
@@ -769,15 +770,13 @@ XenStoreTalkv (
   IN  enum xsd_sockmsg_type   RequestType,
   IN  CONST WRITE_REQUEST     *WriteRequest,
   IN  UINT32                  NumRequests,
-  OUT UINT32                  *LenPtr OPTIONAL,
-  OUT VOID                    **ResultPtr OPTIONAL
+  IN OUT UINTN                *BufferSize OPTIONAL,
+  OUT VOID                    *Buffer OPTIONAL
   )
 {
   struct xsd_sockmsg Message;
   UINTN              Index;
   XENSTORE_STATUS    Status;
-  VOID               *Buffer;
-  UINTN              BufferSize;
 
   if (Transaction == XST_NIL) {
     Message.tx_id = 0;
@@ -805,32 +804,15 @@ XenStoreTalkv (
     }
   }
 
-  if (ResultPtr) {
-    Buffer = AllocatePool (XENSTORE_PAYLOAD_MAX + 1);
-    BufferSize = XENSTORE_PAYLOAD_MAX;
-  } else {
-    Buffer = NULL;
-    BufferSize = 0;
-  }
-
   //
   // Wait for a reply to our request
   //
   Status = XenStoreProcessMessage (Message.req_id, Message.tx_id,
-    &BufferSize, Buffer);
+    BufferSize, Buffer);
 
   if (Status != XENSTORE_STATUS_SUCCESS) {
     DEBUG ((DEBUG_ERROR, "XenStore, error while reading the ring (%d).\n",
         Status));
-    FreePool (Buffer);
-    return Status;
-  }
-
-  if (ResultPtr) {
-    *ResultPtr = Buffer;
-    if (LenPtr) {
-      *LenPtr = BufferSize;
-    }
   }
 
 Error:
@@ -848,8 +830,9 @@ XenStoreTalkv (
   @param RequestType    The type of message to send.
   @param Body           The body of the request.
   @param SubPath        If !NULL and not "", "/$SubPath" is append to Body.
-  @param LenPtr         The returned length of the reply.
-  @param Result         The returned body of the reply.
+  @param BufferSize     IN: sizef of the buffer
+                        OUT: The returned length of the reply.
+  @param Buffer         The returned body of the reply.
 
   @return  0 on success.  Otherwise an errno indicating
            the cause of failure.
@@ -861,8 +844,8 @@ XenStoreSingle (
   IN  enum xsd_sockmsg_type   RequestType,
   IN  CONST CHAR8             *Body,
   IN  CONST CHAR8             *SubPath OPTIONAL,
-  OUT UINT32                  *LenPtr OPTIONAL,
-  OUT VOID                    **Result OPTIONAL
+  IN OUT UINTN                *BufferSize OPTIONAL,
+  OUT VOID                    *Buffer OPTIONAL
   )
 {
   WRITE_REQUEST   WriteRequest[3];
@@ -870,7 +853,7 @@ XenStoreSingle (
   XenStorePrepareWriteRequest (WriteRequest, Body, SubPath);
 
   return XenStoreTalkv (Transaction, RequestType, WriteRequest, 3,
-                        LenPtr, Result);
+    BufferSize, Buffer);
 }
 
 //
@@ -1106,13 +1089,16 @@ XenStoreListDirectory (
   OUT CONST CHAR8           ***DirectoryListPtr
   )
 {
-  CHAR8 *TempStr;
-  UINT32 Len = 0;
+  CHAR8           *TempStr;
+  UINTN           Len;
   XENSTORE_STATUS Status;
 
+  TempStr = AllocatePool (XENSTORE_PAYLOAD_MAX);
+  Len = XENSTORE_PAYLOAD_MAX;
   Status = XenStoreSingle (Transaction, XS_DIRECTORY, DirectoryPath, Node, &Len,
-                           (VOID **) &TempStr);
+    TempStr);
   if (Status != XENSTORE_STATUS_SUCCESS) {
+    FreePool (TempStr);
     return Status;
   }
 
@@ -1146,21 +1132,14 @@ XenStoreRead (
   IN  CONST XENSTORE_TRANSACTION *Transaction,
   IN  CONST CHAR8             *DirectoryPath,
   IN  CONST CHAR8             *Node,
-  OUT UINT32                  *LenPtr OPTIONAL,
-  OUT VOID                    **Result
+  IN OUT UINTN                *BufferSize,
+  OUT VOID                    *Buffer
   )
 {
-  VOID *Value;
-  XENSTORE_STATUS Status;
-
-  Status = XenStoreSingle (Transaction, XS_READ, DirectoryPath, Node,
-    LenPtr, &Value);
-  if (Status != XENSTORE_STATUS_SUCCESS) {
-    return Status;
-  }
-
-  *Result = Value;
-  return XENSTORE_STATUS_SUCCESS;
+  ASSERT (BufferSize != NULL);
+  ASSERT (Buffer != NULL);
+  return XenStoreSingle (Transaction, XS_READ, DirectoryPath, Node,
+    BufferSize, Buffer);
 }
 
 XENSTORE_STATUS
@@ -1199,14 +1178,16 @@ XenStoreTransactionStart (
   OUT XENSTORE_TRANSACTION  *Transaction
   )
 {
-  CHAR8 *IdStr;
+  CHAR8           IdStr[XENSTORE_PAYLOAD_MAX];
+  UINTN           BufferSize;
   XENSTORE_STATUS Status;
 
+  BufferSize = sizeof (IdStr);
+
   Status = XenStoreSingle (XST_NIL, XS_TRANSACTION_START, "", NULL,
-    NULL, (VOID **) &IdStr);
+    &BufferSize, IdStr);
   if (Status == XENSTORE_STATUS_SUCCESS) {
     Transaction->Id = (UINT32)AsciiStrDecimalToUintn (IdStr);
-    FreePool (IdStr);
   }
 
   return Status;
@@ -1358,7 +1339,24 @@ XenBusXenStoreRead (
   OUT VOID                  **Value
   )
 {
-  return XenStoreRead (Transaction, This->Node, Node, NULL, Value);
+  XENSTORE_STATUS Status;
+  UINTN           BufferSize;
+  VOID            *Buffer;
+
+  BufferSize = XENSTORE_PAYLOAD_MAX + 1;
+  Buffer = AllocatePool (BufferSize);
+  if (Buffer == NULL) {
+    return XENSTORE_STATUS_ENOMEM;
+  }
+
+  Status = XenStoreRead (Transaction, This->Node, Node, &BufferSize, Buffer);
+
+  if (Status == XENSTORE_STATUS_SUCCESS) {
+    *Value = Buffer;
+  } else {
+    FreePool (Buffer);
+  }
+  return Status;
 }
 
 XENSTORE_STATUS
@@ -1370,7 +1368,24 @@ XenBusXenStoreBackendRead (
   OUT VOID                  **Value
   )
 {
-  return XenStoreRead (Transaction, This->Backend, Node, NULL, Value);
+  XENSTORE_STATUS Status;
+  UINTN           BufferSize;
+  VOID            *Buffer;
+
+  BufferSize = XENSTORE_PAYLOAD_MAX + 1;
+  Buffer = AllocatePool (BufferSize);
+  if (Buffer == NULL) {
+    return XENSTORE_STATUS_ENOMEM;
+  }
+
+  Status = XenStoreRead (Transaction, This->Backend, Node, &BufferSize, Buffer);
+
+  if (Status == XENSTORE_STATUS_SUCCESS) {
+    *Value = Buffer;
+  } else {
+    FreePool (Buffer);
+  }
+  return Status;
 }
 
 XENSTORE_STATUS
diff --git a/OvmfPkg/XenBusDxe/XenStore.h b/OvmfPkg/XenBusDxe/XenStore.h
index effaad7336..13f7d132e6 100644
--- a/OvmfPkg/XenBusDxe/XenStore.h
+++ b/OvmfPkg/XenBusDxe/XenStore.h
@@ -64,29 +64,26 @@ XenStorePathExists (
   );
 
 /**
-  Get the contents of a single "file".  Returns the contents in *Result which
-  should be freed after use.  The length of the value in bytes is returned in
-  *LenPtr.
+  Get the contents of a single "file".  Copy the contents in Buffer if
+  provided.  The length of the value in bytes is returned in *BufferSize.
 
   @param Transaction    The XenStore transaction covering this request.
   @param DirectoryPath  The dirname of the file to read.
   @param Node           The basename of the file to read.
-  @param LenPtr         The amount of data read.
-  @param Result         The returned contents from this file.
+  @param BufferSize     IN: size of the buffer
+                        OUT: The returned length of the reply.
+  @param Buffer         The returned body of the reply.
 
   @return  On success, XENSTORE_STATUS_SUCCESS. Otherwise an errno value
            indicating the type of failure.
-
-  @note The results buffer is malloced and should be free'd by the
-        caller.
 **/
 XENSTORE_STATUS
 XenStoreRead (
   IN  CONST XENSTORE_TRANSACTION *Transaction,
   IN  CONST CHAR8             *DirectoryPath,
   IN  CONST CHAR8             *Node,
-  OUT UINT32                  *LenPtr OPTIONAL,
-  OUT VOID                    **Result
+  IN OUT UINTN                *BufferSize,
+  OUT VOID                    *Buffer
   );
 
 /**
-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
