[PATCH 1/2] Replace mmalloc() with GNU EFI's AllocatePool()

[Christian Storm]

From: Christian Storm <christi...@siemens.com>

The home-brew mmalloc() is actually superfluous as
GNU EFI's lib/misc.c implements the equivalent
VOID *AllocatePool(UINTN Size)
which is a public API function (inc/efilib.h).

Hence, drop own mmalloc().

Using EFI_MEMORY_TYPE EfiBootServicesData was wrong anyway
as this is not an "EFI boot service driver" (drivers used by
the firmware when booting, PE subsystem 11). It is an "EFI
application" (including OS loaders, PE subsystem 10) which
should use EFI_MEMORY_TYPE EfiLoaderData. This is set
automatically and correctly depending on the PE's subsystem
type in GNU EFI's lib/init.c (follow PoolAllocationType).

Signed-off-by: Christian Storm <christi...@siemens.com>
---
env/fatvars.c | 6 ++++--
include/utils.h | 1 -
utils.c | 24 +++++++-----------------
3 files changed, 11 insertions(+), 20 deletions(-)

diff --git a/env/fatvars.c b/env/fatvars.c
index ea848ff..10593b6 100644
--- a/env/fatvars.c
+++ b/env/fatvars.c
@@ -12,6 +12,8 @@
* SPDX-License-Identifier: GPL-2.0
*/

+#include <efi.h>
+#include <efilib.h>
#include <bootguard.h>
#include <utils.h>
#include <syspart.h>
@@ -27,7 +29,7 @@ BG_STATUS save_current_config(void)
UINTN numHandles = volume_count;
UINTN *config_volumes;

- config_volumes = (UINTN *)mmalloc(sizeof(UINTN) * volume_count);
+ config_volumes = (UINTN *)AllocatePool(sizeof(UINTN) * volume_count);
if (!config_volumes) {
ERROR(L"Could not allocate memory for config partition mapping.\n");
return result;
@@ -90,7 +92,7 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
UINTN i;
int env_invalid[ENV_NUM_CONFIG_PARTS] = {0};

- config_volumes = (UINTN *)mmalloc(sizeof(UINTN) * volume_count);
+ config_volumes = (UINTN *)AllocatePool(sizeof(UINTN) * volume_count);
if (!config_volumes) {
ERROR(L"Could not allocate memory for config partition mapping.\n");
return result;
diff --git a/include/utils.h b/include/utils.h
index 3cac295..eb00a33 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -34,7 +34,6 @@ typedef enum { DOSFSLABEL, CUSTOMLABEL, NOLABEL } LABELMODE;

uint32_t calc_crc32(void *data, int32_t size);
void __attribute__((noreturn)) error_exit(CHAR16 *message, EFI_STATUS status);
-VOID *mmalloc(UINTN bytes);
EFI_STATUS mfree(VOID *p);
CHAR16 *get_volume_label(EFI_FILE_HANDLE fh);
EFI_STATUS get_volumes(VOLUME_DESC **volumes, UINTN *count);
diff --git a/utils.c b/utils.c
index a8c5f6c..0ba22f3 100644
--- a/utils.c
+++ b/utils.c
@@ -13,6 +13,8 @@
* SPDX-License-Identifier: GPL-2.0
*/

+#include <efi.h>
+#include <efilib.h>
#include <bootguard.h>
#include <utils.h>

@@ -63,18 +65,6 @@ void __attribute__((noreturn)) error_exit(CHAR16 *message, EFI_STATUS status)
__builtin_unreachable();
}

-VOID *mmalloc(UINTN bytes)
-{
- EFI_STATUS status;
- VOID *p;
- status = uefi_call_wrapper(BS->AllocatePool, 3, EfiBootServicesData,
- bytes, (VOID **)&p);
- if (EFI_ERROR(status)) {
- return NULL;
- }
- return p;
-}
-
EFI_STATUS mfree(VOID *p)
{
return uefi_call_wrapper(BS->FreePool, 1, p);
@@ -87,7 +77,7 @@ CHAR16 *get_volume_label(EFI_FILE_HANDLE fh)
UINTN fsis;
EFI_STATUS status;

- fsi = mmalloc(MAX_INFO_SIZE);
+ fsi = AllocatePool(MAX_INFO_SIZE);
if (fsi == NULL) {
return NULL;
}
@@ -105,7 +95,7 @@ CHAR16 *get_volume_custom_label(EFI_FILE_HANDLE fh)
{
EFI_STATUS status;
EFI_FILE_HANDLE tmp;
- CHAR16 *buffer = mmalloc(64);
+ CHAR16 *buffer = AllocatePool(64);
UINTN buffsize = 63;

status = uefi_call_wrapper(
@@ -146,7 +136,7 @@ EFI_STATUS get_volumes(VOLUME_DESC **volumes, UINTN *count)
}
INFO(L"Found %d handles for file IO\n\n", handleCount);

- *volumes = (VOLUME_DESC *)mmalloc(sizeof(VOLUME_DESC) * handleCount);
+ *volumes = (VOLUME_DESC *)AllocatePool(sizeof(VOLUME_DESC) * handleCount);
if (!*volumes) {
ERROR(L"Could not allocate memory for volume descriptors.\n");
return EFI_OUT_OF_RESOURCES;
@@ -281,7 +271,7 @@ EFI_DEVICE_PATH *FileDevicePathFromConfig(EFI_HANDLE device,
}

CHAR16 *pathprefix = DevicePathToStr(devpath);
- fullpath = mmalloc(sizeof(CHAR16) *
+ fullpath = AllocatePool(sizeof(CHAR16) *
(StrLen(pathprefix) + StrLen(payloadpath) + 1));

StrCpy(fullpath, pathprefix);
@@ -309,7 +299,7 @@ CHAR16 *GetBootMediumPath(CHAR16 *input)

len = StrLen(input);

- dst = mmalloc((len + 1) * sizeof(CHAR16));
+ dst = AllocatePool((len + 1) * sizeof(CHAR16));
if (!dst) {
return NULL;
}
--
2.30.0

[Christian Storm]

From: Christian Storm <christi...@siemens.com>

The home-brew mfree() is actually superfluous as

GNU EFI's lib/misc.c implements

VOID FreePool(VOID *Buffer)
as also just calling
uefi_call_wrapper(BS->FreePool, 1, Buffer)
and it is a public API function (inc/efilib.h).

FreePool() returns VOID while the home-brew mfree()
returns EFI_STATUS -- which is however never checked
here anyway.
The possible EFI_STATUS return values are:
EFI_SUCCESS - The memory was returned to the system.
EFI_INVALID_PARAMETER - Buffer was invalid.
So it's probably OK to do it as FreePool() does and
ignore the return value.

Hence, drop own mfree().

Signed-off-by: Christian Storm <christi...@siemens.com>
---

env/fatvars.c | 4 ++--
include/utils.h | 1 -
main.c | 6 +++---
utils.c | 21 ++++++++-------------
4 files changed, 13 insertions(+), 19 deletions(-)

diff --git a/env/fatvars.c b/env/fatvars.c
index 10593b6..4cf580d 100644
--- a/env/fatvars.c
+++ b/env/fatvars.c
@@ -80,7 +80,7 @@ BG_STATUS save_current_config(void)

result = BG_SUCCESS;
scc_cleanup:
- mfree(config_volumes);
+ FreePool(config_volumes);
return result;
}

@@ -221,7 +221,7 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
INFO(L" timeout: %d seconds\n", bglp->timeout);

lc_cleanup:
- mfree(config_volumes);
+ FreePool(config_volumes);

return result;
}

diff --git a/include/utils.h b/include/utils.h

index eb00a33..825a647 100644

--- a/include/utils.h
+++ b/include/utils.h
@@ -34,7 +34,6 @@ typedef enum { DOSFSLABEL, CUSTOMLABEL, NOLABEL } LABELMODE;

uint32_t calc_crc32(void *data, int32_t size);
void __attribute__((noreturn)) error_exit(CHAR16 *message, EFI_STATUS status);

-EFI_STATUS mfree(VOID *p);

CHAR16 *get_volume_label(EFI_FILE_HANDLE fh);
EFI_STATUS get_volumes(VOLUME_DESC **volumes, UINTN *count);

EFI_STATUS close_volumes(VOLUME_DESC *volumes, UINTN count);
diff --git a/main.c b/main.c
index 2bbd2ad..682f5c8 100644
--- a/main.c
+++ b/main.c
@@ -126,7 +126,7 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)

tmp = DevicePathToStr(DevicePathFromHandle(loaded_image->DeviceHandle));
boot_medium_path = GetBootMediumPath(tmp);
- mfree(tmp);
+ FreePool(tmp);
INFO(L"Boot medium: %s\n", boot_medium_path);

status = get_volumes(&volumes, &volume_count);
@@ -178,8 +178,8 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
error_exit(L"Could not load specified kernel image.", status);
}

- mfree(payload_dev_path);
- mfree(boot_medium_path);
+ FreePool(payload_dev_path);
+ FreePool(boot_medium_path);

status =
uefi_call_wrapper(BS->OpenProtocol, 6, payload_handle,
diff --git a/utils.c b/utils.c
index 0ba22f3..9f59b1e 100644
--- a/utils.c
+++ b/utils.c
@@ -39,12 +39,12 @@ BOOLEAN IsOnBootMedium(EFI_DEVICE_PATH *dp)

tmp = DevicePathToStr(dp);
device_path = GetBootMediumPath(tmp);
- mfree(tmp);
+ FreePool(tmp);

if (StrCmp(device_path, boot_medium_path) == 0) {
result = TRUE;
}
- mfree(device_path);
+ FreePool(device_path);

return result;
}
@@ -65,11 +65,6 @@ void __attribute__((noreturn)) error_exit(CHAR16 *message, EFI_STATUS status)
__builtin_unreachable();
}

-EFI_STATUS mfree(VOID *p)
-{
- return uefi_call_wrapper(BS->FreePool, 1, p);
-}
-
CHAR16 *get_volume_label(EFI_FILE_HANDLE fh)
{
EFI_FILE_SYSTEM_INFO *fsi;
@@ -85,7 +80,7 @@ CHAR16 *get_volume_label(EFI_FILE_HANDLE fh)
status =
uefi_call_wrapper(fh->GetInfo, 4, fh, &fsiGuid, &fsis, (VOID *)fsi);
if (EFI_ERROR(status) || fsis == 0) {
- mfree(fsi);
+ FreePool(fsi);
return NULL;
}
return fsi->VolumeLabel;
@@ -183,7 +178,7 @@ EFI_STATUS get_volumes(VOLUME_DESC **volumes, UINTN *count)
devpathstr, (*volumes)[rootCount].fslabel,
(*volumes)[rootCount].fscustomlabel);

- mfree(devpathstr);
+ FreePool(devpathstr);

rootCount++;
}
@@ -214,7 +209,7 @@ EFI_STATUS close_volumes(VOLUME_DESC *volumes, UINTN count)
result = EFI_DEVICE_ERROR;
}
}
- mfree(volumes);
+ FreePool(volumes);
return result;
}

@@ -278,8 +273,8 @@ EFI_DEVICE_PATH *FileDevicePathFromConfig(EFI_HANDLE device,
StrCat(fullpath, payloadpath + prefixlen + 3);
INFO(L"Full path for kernel is: %s\n", fullpath);

- mfree(fullpath);
- mfree(pathprefix);
+ FreePool(fullpath);
+ FreePool(pathprefix);

EFI_DEVICE_PATH *filedevpath;
EFI_DEVICE_PATH *appendeddevpath;
@@ -287,7 +282,7 @@ EFI_DEVICE_PATH *FileDevicePathFromConfig(EFI_HANDLE device,
filedevpath = FileDevicePath(NULL, payloadpath + prefixlen + 3);
appendeddevpath = AppendDevicePath(devpath, filedevpath);

- mfree(filedevpath);
+ FreePool(filedevpath);

return appendeddevpath;
}
--
2.30.0

[Jan Kiszka]

Thanks, both applied to next.

Jan

--
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux