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

["Laszlo Ersek" <lersek@redhat.com>]

On 09/13/19 16:50, Anthony PERARD wrote:
> 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(-)
>

Quoting out of order:

> 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.

(1) I assume this -- i.e. error returned -- covers the case when
BufferSize was too small on input. Can you document that? (Or should it
be obvious from elsewhere?)

> -
> -  @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
>    );
>
>  /**
>

> 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);

(2) Since we're touching this -- please fix the indentation.

>
>    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);

(3) Indentation.

>  }
>
>  //
> @@ -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);

(4) Should this AllocatePool() be mentioned in the commit message? (And
the others below, too.)

The function calls Split(), as well, and that one allocates memory too.
Are they not used at ExitBooServices()?


> +  Len = XENSTORE_PAYLOAD_MAX;
>    Status = XenStoreSingle (Transaction, XS_DIRECTORY, DirectoryPath, Node, &Len,
> -                           (VOID **) &TempStr);
> +    TempStr);

(5) Indentation.

>    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);

(6) Indentation.

>    if (Status == XENSTORE_STATUS_SUCCESS) {
>      Transaction->Id = (UINT32)AsciiStrDecimalToUintn (IdStr);

(7) Sorry if I've missed the obvious: what guarantees that IdStr is
NUL-terminated here? In the other functions, we're NUL-terminating
ourselves.

> -    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/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);

(8) Indentation.

>      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';

(9) I think this could be off-by-one. In the other cases, you decrement
the buffer size first, before passing it to XenStoreRead().

Sorry if I got confused.

>
>      Private = AllocateCopyPool (sizeof (*Private), &gXenBusPrivateData);
>      Private->XenBusIo.Type = AsciiStrDup (Type);
>      Private->XenBusIo.Node = AsciiStrDup (DevicePath);
> -    Private->XenBusIo.Backend = BackendPath;
> +    Private->XenBusIo.Backend = AsciiStrDup (BackendPath);

(10) Where is this released?

>      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;
>    }

Thanks
Laszlo