[coreboot-gerrit] Change in coreboot[master]: drivers/amd/agesa: Fix AGESA heap deallocator

[Marc Jones (Code Review)]

Marc Jones has uploaded this change for review. ( https://review.coreboot.org/23309


Change subject: drivers/amd/agesa: Fix AGESA heap deallocator
......................................................................

drivers/amd/agesa: Fix AGESA heap deallocator

Ported from commit e6033ce1

The deallocation was always subtracting the header,
even when it shouldn't. This caused problems for the
allocator where buffer sizes were incorrect and freed
and used buffers could collide.

Fix the deallocation size.

Clear deallocated concatinated buffer header memory.

Fix the initial calculation of the total buffer size
available to be allocated.

Original-Change-Id: I2789ddf72d662f24709dc5d9873741169cc4ef36

Change-Id: Ibac916fcd964adca97a72617428e3d53012e13a1
Signed-off-by: Marc Jones <marcj303 at gmail.com>
---
M src/drivers/amd/agesa/heapmanager.c
1 file changed, 22 insertions(+), 15 deletions(-)



  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/09/23309/1

diff --git a/src/drivers/amd/agesa/heapmanager.c b/src/drivers/amd/agesa/heapmanager.c
index 04775cd..6030ebb 100644
--- a/src/drivers/amd/agesa/heapmanager.c
+++ b/src/drivers/amd/agesa/heapmanager.c
@@ -121,7 +121,9 @@
 		/* Update the remaining free space */
 		FreedNodeOffset = CurrNodeOffset + CurrNodePtr->BufferSize + sizeof(BIOS_BUFFER_NODE);
 		FreedNodePtr = (BIOS_BUFFER_NODE *) (BiosHeapBaseAddr + FreedNodeOffset);
-		FreedNodePtr->BufferSize = AvailableHeapSize - sizeof(BIOS_BUFFER_NODE) - CurrNodePtr->BufferSize;
+		FreedNodePtr->BufferSize = AvailableHeapSize
+					- (FreedNodeOffset - CurrNodeOffset)
+					- sizeof(BIOS_BUFFER_NODE);
 		FreedNodePtr->NextNodeOffset = 0;
 
 		/* Update the offsets for Allocated and Freed nodes */
@@ -250,25 +252,25 @@
 	/* Zero out the buffer, and clear the BufferHandle */
 	LibAmdMemFill ((UINT8 *)AllocNodePtr + sizeof(BIOS_BUFFER_NODE), 0, AllocNodePtr->BufferSize, &(AllocParams->StdHeader));
 	AllocNodePtr->BufferHandle = 0;
-	AllocNodePtr->BufferSize += sizeof(BIOS_BUFFER_NODE);
 
 	/* Add deallocated node in order to the list of freed nodes */
 	FreedNodeOffset = BiosHeapBasePtr->StartOfFreedNodes;
 	FreedNodePtr = (BIOS_BUFFER_NODE *) (BiosHeapBaseAddr + FreedNodeOffset);
 
-	EndNodeOffset = AllocNodeOffset + AllocNodePtr->BufferSize;
+	EndNodeOffset = AllocNodeOffset + AllocNodePtr->BufferSize +
+						sizeof(BIOS_BUFFER_NODE);
 
 	if (AllocNodeOffset < FreedNodeOffset) {
 		/* Add to the start of the freed list */
 		if (EndNodeOffset == FreedNodeOffset) {
 			/* If the freed node is adjacent to the first node in the list, concatenate both nodes */
-			AllocNodePtr->BufferSize += FreedNodePtr->BufferSize;
+			AllocNodePtr->BufferSize += FreedNodePtr->BufferSize +
+						sizeof(BIOS_BUFFER_NODE);
 			AllocNodePtr->NextNodeOffset = FreedNodePtr->NextNodeOffset;
 
-			/* Clear the BufferSize and NextNodeOffset of the previous first node */
-			FreedNodePtr->BufferSize = 0;
-			FreedNodePtr->NextNodeOffset = 0;
-
+			/* Zero out the FreedNode header */
+			memset((UINT8 *)FreedNodePtr, 0,
+						sizeof(BIOS_BUFFER_NODE));
 		} else {
 			/* Otherwise, add freed node to the start of the list
 			 * Update NextNodeOffset and BufferSize to include the
@@ -298,11 +300,13 @@
 		 */
 		if (NextNodeOffset == EndNodeOffset) {
 			NextNodePtr = (BIOS_BUFFER_NODE *) (BiosHeapBaseAddr + NextNodeOffset);
-			AllocNodePtr->BufferSize += NextNodePtr->BufferSize;
+			AllocNodePtr->BufferSize += NextNodePtr->BufferSize +
+						sizeof(BIOS_BUFFER_NODE);
 			AllocNodePtr->NextNodeOffset = NextNodePtr->NextNodeOffset;
 
-			NextNodePtr->BufferSize = 0;
-			NextNodePtr->NextNodeOffset = 0;
+			/* Zero out the NextNode header */
+			memset((UINT8 *)NextNodePtr, 0,
+						sizeof(BIOS_BUFFER_NODE));
 		} else {
 			/*AllocNodePtr->NextNodeOffset = FreedNodePtr->NextNodeOffset; */
 			AllocNodePtr->NextNodeOffset = NextNodeOffset;
@@ -311,13 +315,16 @@
 		 * concatenate both nodes.
 		 */
 		PrevNodePtr = (BIOS_BUFFER_NODE *) (BiosHeapBaseAddr + PrevNodeOffset);
-		EndNodeOffset = PrevNodeOffset + PrevNodePtr->BufferSize;
+		EndNodeOffset = PrevNodeOffset + PrevNodePtr->BufferSize +
+						sizeof(BIOS_BUFFER_NODE);
 		if (AllocNodeOffset == EndNodeOffset) {
 			PrevNodePtr->NextNodeOffset = AllocNodePtr->NextNodeOffset;
-			PrevNodePtr->BufferSize += AllocNodePtr->BufferSize;
+			PrevNodePtr->BufferSize += AllocNodePtr->BufferSize +
+						sizeof(BIOS_BUFFER_NODE)
 
-			AllocNodePtr->BufferSize = 0;
-			AllocNodePtr->NextNodeOffset = 0;
+			/* Zero out the AllocNode header */
+			memset((UINT8 *)AllocNodePtr, 0,
+						sizeof(BIOS_BUFFER_NODE));
 		} else {
 			PrevNodePtr->NextNodeOffset = AllocNodeOffset;
 		}

-- 
To view, visit https://review.coreboot.org/23309
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibac916fcd964adca97a72617428e3d53012e13a1
Gerrit-Change-Number: 23309
Gerrit-PatchSet: 1
Gerrit-Owner: Marc Jones <marc at marcjonesconsulting.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20180117/ff9af31f/attachment-0001.html>