From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Mon, 16 Sep 2019 09:16:43 -0700 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 54FFB3175298; Mon, 16 Sep 2019 16:16:43 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-124-96.rdu2.redhat.com [10.10.124.96]) by smtp.corp.redhat.com (Postfix) with ESMTP id B08056012A; Mon, 16 Sep 2019 16:16:41 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 08/11] OvmfPkg/XenBus: Change XENBUS_PROTOCOL to not return allocated memory To: devel@edk2.groups.io, anthony.perard@citrix.com Cc: Ard Biesheuvel , Julien Grall , Jordan Justen , xen-devel@lists.xenproject.org References: <20190913145100.303433-1-anthony.perard@citrix.com> <20190913145100.303433-9-anthony.perard@citrix.com> From: "Laszlo Ersek" Message-ID: <6c8ab94a-cc2b-9354-207c-620a3939e199@redhat.com> Date: Mon, 16 Sep 2019 18:16:40 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190913145100.303433-9-anthony.perard@citrix.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.49]); Mon, 16 Sep 2019 16:16:43 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 09/13/19 16:50, Anthony PERARD wrote: > XsRead and XsBackendRead of the XENBUS_PROTOCOL return allocated > memory but this isn't allowed during the ExitBootServices call. We > need XsRead and XsBackendRead to disconnect from the device so > XENBUS_PROTOCOL is changed to use a buffer supplied by a child driver. > > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2190 > Signed-off-by: Anthony PERARD > --- > OvmfPkg/Include/Protocol/XenBus.h | 32 ++++++++++++---------- > OvmfPkg/XenBusDxe/XenStore.c | 44 +++++------------------------- > OvmfPkg/XenBusDxe/XenStore.h | 6 +++-- > OvmfPkg/XenPvBlkDxe/BlockFront.c | 45 ++++++++++++++++++------------- > 4 files changed, 54 insertions(+), 73 deletions(-) > > diff --git a/OvmfPkg/Include/Protocol/XenBus.h b/OvmfPkg/Include/Protocol/XenBus.h > index 8ff5ca3575..c22bdfb368 100644 > --- a/OvmfPkg/Include/Protocol/XenBus.h > +++ b/OvmfPkg/Include/Protocol/XenBus.h > @@ -35,6 +35,12 @@ typedef struct > > #define XST_NIL ((XENSTORE_TRANSACTION *) NULL) > > +// > +// When reading a node from xenstore, if the size of the data to be read is > +// unknown, this value can be use for the size of the buffer. > +// > +#define XENSTORE_PAYLOAD_MAX 4096 > + This macro is already defined in "IndustryStandard/Xen/io/xs_wire.h". Can we get it from there? The extra documentation is OK, of course (replacing "this value" with "XENSTORE_PAYLOAD_MAX"). Other than that, I'm going to have to ACK this after a brief skim only. Acked-by: Laszlo Ersek Thanks Laszlo > typedef enum { > XENSTORE_STATUS_SUCCESS = 0, > XENSTORE_STATUS_FAIL, > @@ -64,19 +70,17 @@ typedef enum { > /// > > /** > - Get the contents of the node Node of the PV device. Returns the contents in > - *Result which should be freed after use. > + Get the contents of the node Node of the PV device. > > @param This A pointer to XENBUS_PROTOCOL instance. > @param Transaction The XenStore transaction covering this request. > @param Node The basename of the file to read. > - @param Result The returned contents from this file. > + @param BufferSize On input, a pointer to the size of the buffer at Buffer. > + On output, the size of the data written to Buffer. > + @param Buffer A pointer to a buffer into which the data read will be saved. > > @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. > **/ > typedef > XENSTORE_STATUS > @@ -84,23 +88,22 @@ XENSTORE_STATUS > IN XENBUS_PROTOCOL *This, > IN CONST XENSTORE_TRANSACTION *Transaction, > IN CONST CHAR8 *Node, > - OUT VOID **Result > + IN OUT UINTN *BufferSize, > + OUT VOID *Buffer > ); > > /** > - Get the contents of the node Node of the PV device's backend. Returns the > - contents in *Result which should be freed after use. > + Get the contents of the node Node of the PV device's backend. > > @param This A pointer to XENBUS_PROTOCOL instance. > @param Transaction The XenStore transaction covering this request. > @param Node The basename of the file to read. > - @param Result The returned contents from this file. > + @param BufferSize On input, a pointer to the size of the buffer at Buffer. > + On output, the size of the data written to Buffer. > + @param Buffer A pointer to a buffer into which the data read will be saved. > > @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. > **/ > typedef > XENSTORE_STATUS > @@ -108,7 +111,8 @@ XENSTORE_STATUS > IN XENBUS_PROTOCOL *This, > IN CONST XENSTORE_TRANSACTION *Transaction, > IN CONST CHAR8 *Node, > - OUT VOID **Result > + IN OUT UINTN *BufferSize, > + OUT VOID *Buffer > ); > > /** > diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c > index b9588bb8c6..cb2d9e1215 100644 > --- a/OvmfPkg/XenBusDxe/XenStore.c > +++ b/OvmfPkg/XenBusDxe/XenStore.c > @@ -1336,27 +1336,11 @@ XenBusXenStoreRead ( > IN XENBUS_PROTOCOL *This, > IN CONST XENSTORE_TRANSACTION *Transaction, > IN CONST CHAR8 *Node, > - OUT VOID **Value > + IN OUT UINTN *BufferSize, > + OUT VOID *Buffer > ) > { > - 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; > + return XenStoreRead (Transaction, This->Node, Node, BufferSize, Buffer); > } > > XENSTORE_STATUS > @@ -1365,27 +1349,11 @@ XenBusXenStoreBackendRead ( > IN XENBUS_PROTOCOL *This, > IN CONST XENSTORE_TRANSACTION *Transaction, > IN CONST CHAR8 *Node, > - OUT VOID **Value > + IN OUT UINTN *BufferSize, > + OUT VOID *Buffer > ) > { > - 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; > + return XenStoreRead (Transaction, This->Backend, Node, BufferSize, Buffer); > } > > XENSTORE_STATUS > diff --git a/OvmfPkg/XenBusDxe/XenStore.h b/OvmfPkg/XenBusDxe/XenStore.h > index 13f7d132e6..ca8c080433 100644 > --- a/OvmfPkg/XenBusDxe/XenStore.h > +++ b/OvmfPkg/XenBusDxe/XenStore.h > @@ -289,7 +289,8 @@ XenBusXenStoreRead ( > IN XENBUS_PROTOCOL *This, > IN CONST XENSTORE_TRANSACTION *Transaction, > IN CONST CHAR8 *Node, > - OUT VOID **Value > + IN OUT UINTN *BufferSize, > + OUT VOID *Buffer > ); > > XENSTORE_STATUS > @@ -298,7 +299,8 @@ XenBusXenStoreBackendRead ( > IN XENBUS_PROTOCOL *This, > IN CONST XENSTORE_TRANSACTION *Transaction, > IN CONST CHAR8 *Node, > - OUT VOID **Value > + IN OUT UINTN *BufferSize, > + OUT VOID *Buffer > ); > > XENSTORE_STATUS > diff --git a/OvmfPkg/XenPvBlkDxe/BlockFront.c b/OvmfPkg/XenPvBlkDxe/BlockFront.c > index 8dca4c82f0..25a398ccc4 100644 > --- a/OvmfPkg/XenPvBlkDxe/BlockFront.c > +++ b/OvmfPkg/XenPvBlkDxe/BlockFront.c > @@ -41,19 +41,22 @@ XenBusReadUint64 ( > ) > { > XENSTORE_STATUS Status; > - CHAR8 *Ptr; > + CHAR8 Buffer[22]; > + UINTN BufferSize; > + > + BufferSize = sizeof (Buffer) - 1; > > if (!FromBackend) { > - Status = This->XsRead (This, XST_NIL, Node, (VOID**)&Ptr); > + Status = This->XsRead (This, XST_NIL, Node, &BufferSize, Buffer); > } else { > - Status = This->XsBackendRead (This, XST_NIL, Node, (VOID**)&Ptr); > + Status = This->XsBackendRead (This, XST_NIL, Node, &BufferSize, Buffer); > } > if (Status != XENSTORE_STATUS_SUCCESS) { > return Status; > } > + Buffer[BufferSize] = '\0'; > // AsciiStrDecimalToUint64 will ASSERT if Ptr overflow UINT64. > - *ValuePtr = AsciiStrDecimalToUint64 (Ptr); > - FreePool (Ptr); > + *ValuePtr = AsciiStrDecimalToUint64 (Buffer); > return Status; > } > > @@ -143,43 +146,47 @@ XenPvBlockFrontInitialization ( > OUT XEN_BLOCK_FRONT_DEVICE **DevPtr > ) > { > - XENSTORE_TRANSACTION Transaction; > - CHAR8 *DeviceType; > - blkif_sring_t *SharedRing; > - XENSTORE_STATUS Status; > + XENSTORE_TRANSACTION Transaction; > + CHAR8 Buffer[XENSTORE_PAYLOAD_MAX + 1]; > + UINTN BufferSize; > + blkif_sring_t *SharedRing; > + XENSTORE_STATUS Status; > XEN_BLOCK_FRONT_DEVICE *Dev; > - XenbusState State; > - UINT64 Value; > - CHAR8 *Params; > + XenbusState State; > + UINT64 Value; > > ASSERT (NodeName != NULL); > > + BufferSize = sizeof (Buffer) - 1; > + > Dev = AllocateZeroPool (sizeof (XEN_BLOCK_FRONT_DEVICE)); > Dev->Signature = XEN_BLOCK_FRONT_SIGNATURE; > Dev->NodeName = NodeName; > Dev->XenBusIo = XenBusIo; > Dev->DeviceId = XenBusIo->DeviceId; > > - XenBusIo->XsRead (XenBusIo, XST_NIL, "device-type", (VOID**)&DeviceType); > - if (AsciiStrCmp (DeviceType, "cdrom") == 0) { > + BufferSize = sizeof (Buffer) - 1; > + XenBusIo->XsRead (XenBusIo, XST_NIL, "device-type", &BufferSize, Buffer); > + Buffer[BufferSize] = '\0'; > + if (AsciiStrCmp (Buffer, "cdrom") == 0) { > Dev->MediaInfo.CdRom = TRUE; > } else { > Dev->MediaInfo.CdRom = FALSE; > } > - FreePool (DeviceType); > > if (Dev->MediaInfo.CdRom) { > - Status = XenBusIo->XsBackendRead (XenBusIo, XST_NIL, "params", (VOID**)&Params); > + BufferSize = sizeof (Buffer) - 1; > + Status = XenBusIo->XsBackendRead (XenBusIo, XST_NIL, "params", > + &BufferSize, Buffer); > if (Status != XENSTORE_STATUS_SUCCESS) { > DEBUG ((EFI_D_ERROR, "%a: Failed to read params (%d)\n", __FUNCTION__, Status)); > goto Error; > } > - if (AsciiStrLen (Params) == 0 || AsciiStrCmp (Params, "aio:") == 0) { > - FreePool (Params); > + Buffer[BufferSize] = '\0'; > + if (AsciiStrLen (Buffer) == 0 || AsciiStrCmp (Buffer, "aio:") == 0) { > DEBUG ((EFI_D_INFO, "%a: Empty cdrom\n", __FUNCTION__)); > goto Error; > } > - FreePool (Params); > } > > Status = XenBusReadUint64 (XenBusIo, "backend-id", FALSE, &Value); >