From ed17f9fdff111a40318aaeb206c65a685ccc4c5a Mon Sep 17 00:00:00 2001 From: Peter Jones Date: Tue, 5 Mar 2024 16:31:24 -0500 Subject: [PATCH 1/9] safemath: fix users of is_null() The safe math code was largely copied from another project, and that project had an is_null() function that we don't have. This patch just makes its two users a normal NULL check. Related: RHEL-27676 Signed-off-by: Peter Jones --- src/safemath.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/safemath.h b/src/safemath.h index caa5b1cc..8d98fb61 100644 --- a/src/safemath.h +++ b/src/safemath.h @@ -32,7 +32,7 @@ #define DIV(a, b, res) ({ \ bool ret_ = true; \ if ((b) != 0) { \ - if (!is_null(res)) \ + if (!(res == NULL)) \ (*(res)) = (a) / (b); \ ret_ = false; \ } else { \ @@ -44,7 +44,7 @@ * These really just exists for chaining results easily with || in an expr */ #define MOD(a, b, res) ({ \ - if (!is_null(res)) \ + if (!(res == NULL)) \ (*(res)) = (a) % (b); \ false; \ }) From 606faed7a807374569405b13dcc787e43e15718a Mon Sep 17 00:00:00 2001 From: Peter Jones Date: Tue, 5 Mar 2024 16:32:36 -0500 Subject: [PATCH 2/9] Fix wrong variadic initialization One of our internal scanning tools found the following error: Error: VARARGS (CWE-237): efivar-38/src/creator.c:227: va_arg: Calling va_arg on va_list "aq", which has not been prepared with va_start(). # 225| va_copy(aq, ap); # 226| # 227|-> dev->edd10_devicenum = va_arg(aq, uint32_t); # 228| # 229| va_end(aq); I have no idea what the code in efi_va_generate_file_device_path_from_esp() was trying to do, but as far as I can tell we shouldn't need to do anything special to access the variadic arguments. This patch takes most of that code out. Resolves: RHEL-27676 Signed-off-by: Peter Jones --- src/creator.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/creator.c b/src/creator.c index e62188ce..f780a80b 100644 --- a/src/creator.c +++ b/src/creator.c @@ -221,12 +221,7 @@ efi_va_generate_file_device_path_from_esp(uint8_t *buf, ssize_t size, debug("EFIBOOT_ABBREV_EDD10"); if (options & EFIBOOT_ABBREV_EDD10) { - va_list aq; - va_copy(aq, ap); - - dev->edd10_devicenum = va_arg(aq, uint32_t); - - va_end(aq); + dev->edd10_devicenum = va_arg(ap, uint32_t); } if (!(options & (EFIBOOT_ABBREV_FILE|EFIBOOT_ABBREV_HD)) From 8a24122bcb39fdd589f8c4fd23e98f79b49aeda6 Mon Sep 17 00:00:00 2001 From: Leo Sandoval Date: Tue, 5 Mar 2024 16:32:36 -0500 Subject: [PATCH 3/9] src/dp-media.c: fix overruns on efidp_node_size calls The Static Application Security Testing (SAST) found possible memory buffer overruns when calling the function `efidp_node_size` where it may return a negative value and it's being used without check on buffer indexation. The proposed fix check the return value before usage. Below is the SAST log which may be useful for future searches: "Error: OVERRUN (CWE-119): efivar-38/src/dp-media.c:61: return_constant: Function call ""efidp_node_size(dp)"" may return -1. efivar-38/src/dp-media.c:61: assignment: Assigning: ""limit"" = ""(efidp_node_size(dp) - 4UL) / 2UL"". The value of ""limit"" is now 9223372036854775805. efivar-38/src/dp-media.c:64: overrun-buffer-arg: Calling ""memcpy"" with ""_asciibuf"" and ""limit"" is suspicious because of the very large index, 9223372036854775805. The index may be due to a negative parameter being interpreted as unsigned. [Note: The source code implementation of the function has been overridden by a builtin model.] # 62| - offsetof(efidp_file, name)) / 2; # 63| format(buf, size, off, ""File"", ""File(""); # 64|-> format_ucs2(buf, size, off, ""File"", dp->file.name, limit); # 65| format(buf, size, off, ""File"", "")""); # 66| break;" Resolves: RHEL-27676 Signed-off-by: Leo Sandoval --- src/dp-media.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/dp-media.c b/src/dp-media.c index ffda361b..e5770217 100644 --- a/src/dp-media.c +++ b/src/dp-media.c @@ -58,9 +58,14 @@ _format_media_dn(unsigned char *buf, size_t size, const_efidp dp) format_vendor(buf, size, off, "VenMedia", dp); break; case EFIDP_MEDIA_FILE: { - size_t limit = (efidp_node_size(dp) - - offsetof(efidp_file, name)) / 2; - format_ucs2(buf, size, off, "File", dp->file.name, limit); + ssize_t node_size = efidp_node_size(dp); + if (node_size > 0) { + size_t limit = ((size_t) node_size + - offsetof(efidp_file, name)) / 2; + format_ucs2(buf, size, off, "File", dp->file.name, limit); + } else { + efi_error("Invalid node size"); + } break; } case EFIDP_MEDIA_PROTOCOL: From 812f20ba6f13e465df59d7f72185bd1eb4cfb09c Mon Sep 17 00:00:00 2001 From: Peter Jones Date: Wed, 6 Mar 2024 10:27:49 -0500 Subject: [PATCH 4/9] dp-media: more efidp_node_size() fixups. In a prior commit, Leo fixed one of _format_media_dn()'s invocations of efidp_node_size() to handle the failure case. This patch builds on that, using our more robust math primitives in that case. There's also a second analogous issue: Error: OVERRUN (CWE-119): efivar-38/src/dp-media.c:133: return_constant: Function call "efidp_node_size(dp)" may return -1. efivar-38/src/dp-media.c:133: overrun-buffer-arg: Calling "format_hex_helper" with "(uint8_t *)dp + 4" and "(efidp_node_size(dp) - 4L) / 2L" is suspicious because of the very large index, 18446744073709551614. The index may be due to a negative parameter being interpreted as unsigned. # 131| default: # 132| format(buf, size, off, "Media", "MediaPath(%d,", dp->subtype); # 133|-> format_hex(buf, size, off, "Media", (uint8_t *)dp+4, # 134| (efidp_node_size(dp)-4) / 2); # 135| format(buf,size,off, "Media",")"); This patch fixes that case as well. Resolves: RHEL-27676 Signed-off-by: Peter Jones --- src/dp-media.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/src/dp-media.c b/src/dp-media.c index e5770217..f8b1c490 100644 --- a/src/dp-media.c +++ b/src/dp-media.c @@ -58,14 +58,15 @@ _format_media_dn(unsigned char *buf, size_t size, const_efidp dp) format_vendor(buf, size, off, "VenMedia", dp); break; case EFIDP_MEDIA_FILE: { - ssize_t node_size = efidp_node_size(dp); - if (node_size > 0) { - size_t limit = ((size_t) node_size - - offsetof(efidp_file, name)) / 2; - format_ucs2(buf, size, off, "File", dp->file.name, limit); - } else { - efi_error("Invalid node size"); + ssize_t limit = efidp_node_size(dp); + size_t offset = offsetof(efidp_usb_wwid, serial_number); + if (limit < 0 || + SUB(limit, offset, &limit) || + DIV(limit, 2, &limit)) { + efi_error("bad DP node size"); + return -1; } + format_ucs2(buf, size, off, "File", dp->file.name, limit); break; } case EFIDP_MEDIA_PROTOCOL: @@ -131,12 +132,19 @@ _format_media_dn(unsigned char *buf, size_t size, const_efidp dp) format(buf, size, off, "Ramdisk", ")"); break; } - default: + default: { + ssize_t limit = efidp_node_size(dp); + if (limit < 0 || + SUB(limit, 4, &limit) || + DIV(limit, 2, &limit)) { + efi_error("bad DP node size"); + return -1; + } format(buf, size, off, "Media", "MediaPath(%d,", dp->subtype); - format_hex(buf, size, off, "Media", (uint8_t *)dp+4, - (efidp_node_size(dp)-4) / 2); + format_hex(buf, size, off, "Media", (uint8_t *)dp+4, limit); format(buf,size,off, "Media",")"); break; + } } return off; } From d0fa2b8a5b9c010d8dd615efe2592ab3ec6e8a1f Mon Sep 17 00:00:00 2001 From: Peter Jones Date: Wed, 6 Mar 2024 10:38:13 -0500 Subject: [PATCH 5/9] dp.h: Fix incorrect efidp_node_size() error handling One of our internal analysis tools noticed the following: Error: OVERRUN (CWE-119): efivar-38/src/dp.h:104: return_constant: Function call "efidp_node_size(dp)" may return -1. efivar-38/src/dp.h:104: assignment: Assigning: "bytes" = "efidp_node_size(dp) - 4UL - 16UL". The value of "bytes" is now -21. efivar-38/src/dp.h:112: overrun-buffer-arg: Calling "format_hex_helper" with "dp->hw_vendor.vendor_data" and "bytes" is suspicious because of the very large index, 18446744073709551595. The index may be due to a negative parameter being interpreted as unsigned. # 110| if (bytes) { # 111| format(buf, size, off, label, ","); # 112|-> format_hex(buf, size, off, label, dp->hw_vendor.vendor_data, # 113| bytes); # 114| } This patch corrects the error checking for the efidp_node_size() invocation, so the code will never get this far if it errors. Resolves: RHEL-27676 Signed-off-by: Peter Jones --- src/dp.h | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/dp.h b/src/dp.h index 2a7b231d..59f436a0 100644 --- a/src/dp.h +++ b/src/dp.h @@ -101,9 +101,14 @@ format_vendor_helper(unsigned char *buf, size_t size, char *label, const_efidp dp) { ssize_t off = 0; - ssize_t bytes = efidp_node_size(dp) - - sizeof (efidp_header) - - sizeof (efi_guid_t); + ssize_t bytes = efidp_node_size(dp); + + if (SUB(bytes, sizeof (efidp_header), &bytes) || + SUB(bytes, sizeof (efi_guid_t), &bytes) || + bytes < 0) { + efi_error("bad DP node size"); + return -1; + } format(buf, size, off, label, "%s(", label); format_guid(buf, size, off, label, &dp->hw_vendor.vendor_guid); From 400bad72c15187e2d75fc5f69bec6788d6072236 Mon Sep 17 00:00:00 2001 From: Peter Jones Date: Wed, 6 Mar 2024 10:51:19 -0500 Subject: [PATCH 6/9] efidp_format_device_path(): fix efidp_node_size() return value handling. One of our internal scan tools noticed the following issues: Error: OVERRUN (CWE-119): efivar-38/src/dp.c:342: return_constant: Function call "efidp_node_size(dp)" may return -1. efivar-38/src/dp.c:342: overrun-buffer-arg: Calling "format_hex_helper" with "(uint8_t *)dp + 4" and "efidp_node_size(dp) - 4L" is suspicious because of the very large index, 18446744073709551611. The index may be due to a negative parameter being interpreted as unsigned. # 340| format(buf, size, off, "BbsPath", # 341| "BbsPath(%d,", dp->subtype); # 342|-> format_hex(buf, size, off, "BbsPath", # 343| (uint8_t *)dp+4, # 344| efidp_node_size(dp)-4); Error: OVERRUN (CWE-119): efivar-38/src/dp.c:374: return_constant: Function call "efidp_node_size(dp)" may return -1. efivar-38/src/dp.c:374: overrun-buffer-arg: Calling "format_hex_helper" with "(uint8_t *)dp + 4" and "efidp_node_size(dp) - 4L" is suspicious because of the very large index, 18446744073709551611. The index may be due to a negative parameter being interpreted as unsigned. # 372| format(buf, size, off, "Path", # 373| "Path(%d,%d,", dp->type, dp->subtype); # 374|-> format_hex(buf, size, off, "Path", (uint8_t *)dp + 4, # 375| efidp_node_size(dp) - 4); # 376| format(buf, size, off, "Path", ")"); These are both in the same device path traversal loop. This patch refactors that code to do the math and the error check at the top of the loop. Resolves: RHEL-27676 Signed-off-by: Peter Jones --- src/dp.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/dp.c b/src/dp.c index 0dc7492a..83acf9ee 100644 --- a/src/dp.c +++ b/src/dp.c @@ -298,6 +298,12 @@ efidp_format_device_path(unsigned char *buf, size_t size, const_efidp dp, memset(buf, 0, size); while (limit) { + ssize_t sz = efidp_node_size(dp); + if (SUB(sz, 4, &sz) || + sz < 0) { + efi_error("bad DP node size"); + return -1; + } if (limit >= 0 && (limit < 4 || efidp_node_size(dp) > limit)) { if (off) return off; @@ -339,9 +345,9 @@ efidp_format_device_path(unsigned char *buf, size_t size, const_efidp dp, if (dp->subtype != EFIDP_BIOS_BOOT) { format(buf, size, off, "BbsPath", "BbsPath(%d,", dp->subtype); - format_hex(buf, size, off, "BbsPath", - (uint8_t *)dp+4, - efidp_node_size(dp)-4); + if (sz > 0) + format_hex(buf, size, off, "BbsPath", + (uint8_t *)dp+4, sz); format(buf, size, off, "BbsPath", ")"); break; } @@ -371,8 +377,9 @@ efidp_format_device_path(unsigned char *buf, size_t size, const_efidp dp, default: format(buf, size, off, "Path", "Path(%d,%d,", dp->type, dp->subtype); + if (sz > 0) format_hex(buf, size, off, "Path", (uint8_t *)dp + 4, - efidp_node_size(dp) - 4); + sz); format(buf, size, off, "Path", ")"); break; } From 57d102392817071680345da8536e46bfbbab3a88 Mon Sep 17 00:00:00 2001 From: Peter Jones Date: Wed, 6 Mar 2024 11:01:20 -0500 Subject: [PATCH 7/9] dp-message: fix efidp_node_size() error checking One of our internal analysis tools noticed the following problems: Error: OVERRUN (CWE-119): efivar-38/src/dp-message.c:481: return_constant: Function call "efidp_node_size(dp)" may return -1. efivar-38/src/dp-message.c:481: assignment: Assigning: "limit" = "(efidp_node_size(dp) - 10UL) / 2UL". The value of "limit" is now 9223372036854775802. efivar-38/src/dp-message.c:488: overrun-buffer-arg: Calling "memcpy" with "_asciibuf" and "limit" is suspicious because of the very large index, 9223372036854775802. The index may be due to a negative parameter being interpreted as unsigned. [Note: The source code implementation of the function has been overridden by a builtin model.] # 486| dp->usb_wwid.vendor_id, dp->usb_wwid.product_id, # 487| dp->usb_wwid.interface); # 488|-> format_ucs2(buf, size, off, "UsbWwid", # 489| dp->usb_wwid.serial_number, limit); # 490| format(buf, size, off, "UsbWwid", ")"); Error: OVERRUN (CWE-119): efivar-38/src/dp-message.c:612: return_constant: Function call "efidp_node_size(dp)" may return -1. efivar-38/src/dp-message.c:612: overrun-buffer-arg: Calling "format_hex_helper" with "(uint8_t *)dp + 4" and "efidp_node_size(dp) - 4L" is suspicious because of the very large index, 18446744073709551611. The index may be due to a negative parameter being interpreted as unsigned. # 610| default: # 611| format(buf, size, off, "Msg", "Msg(%d,", dp->subtype); # 612|-> format_hex(buf, size, off, "Msg", (uint8_t *)dp+4, # 613| efidp_node_size(dp)-4); # 614| format(buf, size, off, "Msg", ")"); This patch adds error checking to those uses of efidp_node_size(). Resolves: RHEL-27676 Signed-off-by: Peter Jones --- src/dp-message.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/src/dp-message.c b/src/dp-message.c index bad4a3cb..464fe0f9 100644 --- a/src/dp-message.c +++ b/src/dp-message.c @@ -504,9 +504,14 @@ _format_message_dn(unsigned char *buf, size_t size, const_efidp dp) format_helper(format_usb_class, buf, size, off, "UsbClass", dp); break; case EFIDP_MSG_USB_WWID: { - size_t limit = (efidp_node_size(dp) - - offsetof(efidp_usb_wwid, serial_number)) - / 2; + ssize_t limit = efidp_node_size(dp); + size_t offset = offsetof(efidp_usb_wwid, serial_number); + if (limit <= 0 || + SUB(limit, offset, &limit) || + DIV(limit, 2, &limit)) { + efi_error("bad DP node size"); + return -1; + } format(buf, size, off, "UsbWwid", "UsbWwid(%"PRIx16",%"PRIx16",%d,", dp->usb_wwid.vendor_id, dp->usb_wwid.product_id, @@ -633,12 +638,18 @@ _format_message_dn(unsigned char *buf, size_t size, const_efidp dp) format_guid(buf, size, off, "NVDIMM", &dp->nvdimm.uuid); format(buf, size, off, "NVDIMM", ")"); break; - default: + default: { + ssize_t limit = efidp_node_size(dp); + if (SUB(limit, 4, &limit) || + limit < 0) { + efi_error("bad DP node size"); + return -1; + } format(buf, size, off, "Msg", "Msg(%d,", dp->subtype); - format_hex(buf, size, off, "Msg", (uint8_t *)dp+4, - efidp_node_size(dp)-4); + format_hex(buf, size, off, "Msg", (uint8_t *)dp+4, limit); format(buf, size, off, "Msg", ")"); break; + } } return off; } From 4e0901511ecfb519d4ddf780c22dc00e1046c556 Mon Sep 17 00:00:00 2001 From: Peter Jones Date: Wed, 6 Mar 2024 11:15:16 -0500 Subject: [PATCH 8/9] dp-acpi.c: fix incorrect error handling of efidp_node_size() invocations One of our analysis tools noticed the following error: Error: OVERRUN (CWE-119): efivar-38/src/dp-acpi.c:106: return_constant: Function call "efidp_node_size(dp)" may return -1. efivar-38/src/dp-acpi.c:106: overrun-buffer-arg: Calling "format_hex_helper" with "(uint8_t *)dp + 4" and "(efidp_node_size(dp) - 4L) / 2L" is suspicious because of the very large index, 18446744073709551614. The index may be due to a negative parameter being interpreted as unsigned. # 104| debug("DP subtype %d, formatting as ACPI Path", dp->subtype); # 105| format(buf, size, off, "AcpiPath", "AcpiPath(%d,", dp->subtype); # 106|-> format_hex(buf, size, off, "AcpiPath", (uint8_t *)dp+4, # 107| (efidp_node_size(dp)-4) / 2); # 108| format(buf, size, off, "AcpiPath", ")"); This patch adds error checking to that use of efidp_node_size(). Resolves: RHEL-27676 Signed-off-by: Peter Jones --- src/dp-acpi.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/dp-acpi.c b/src/dp-acpi.c index c9d15862..ff79fd4a 100644 --- a/src/dp-acpi.c +++ b/src/dp-acpi.c @@ -101,10 +101,17 @@ _format_acpi_dn(unsigned char *buf, size_t size, const_efidp dp) return off; } else if (dp->subtype != EFIDP_ACPI_HID_EX && dp->subtype != EFIDP_ACPI_HID) { + ssize_t limit = efidp_node_size(dp); + debug("DP subtype %d, formatting as ACPI Path", dp->subtype); + if (SUB(limit, 4, &limit) || + DIV(limit, 2, &limit) || + limit < 0) { + efi_error("bad DP node size"); + return -1; + } format(buf, size, off, "AcpiPath", "AcpiPath(%d,", dp->subtype); - format_hex(buf, size, off, "AcpiPath", (uint8_t *)dp+4, - (efidp_node_size(dp)-4) / 2); + format_hex(buf, size, off, "AcpiPath", (uint8_t *)dp+4, limit); format(buf, size, off, "AcpiPath", ")"); return off; } else if (dp->subtype == EFIDP_ACPI_HID_EX) { From b580d3baca6cdc172dfa85a4eb6438b6846d406c Mon Sep 17 00:00:00 2001 From: Peter Jones Date: Wed, 6 Mar 2024 11:15:16 -0500 Subject: [PATCH 9/9] dp-hw.c: fix incorrect error handling of efidp_node_size() invocations One of our analysis tools noticed the following error: Error: OVERRUN (CWE-119): efivar-38/src/dp-hw.c:64: return_constant: Function call "efidp_node_size(dp)" may return -1. efivar-38/src/dp-hw.c:64: overrun-buffer-arg: Calling "format_hex_helper" with "(uint8_t *)dp + 4" and "efidp_node_size(dp) - 4L" is suspicious because of the very large index, 18446744073709551611. The index may be due to a negative parameter being interpreted as unsigned. # 62| format(buf, size, off, "Hardware", # 63| "HardwarePath(%d,", dp->subtype); # 64|-> format_hex(buf, size, off, "Hardware", (uint8_t *)dp+4, # 65| efidp_node_size(dp)-4); # 66| format(buf, size, off, "Hardware", ")"); This patch adds error checking to that use of efidp_node_size(). Resolves: RHEL-27676 Signed-off-by: Peter Jones --- src/dp-hw.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/dp-hw.c b/src/dp-hw.c index e09984db..e94cf7bc 100644 --- a/src/dp-hw.c +++ b/src/dp-hw.c @@ -58,13 +58,20 @@ _format_hw_dn(unsigned char *buf, size_t size, const_efidp dp) format(buf, size, off, "BMC", "BMC(%d,0x%"PRIx64")", dp->bmc.interface_type, dp->bmc.base_addr); break; - default: + default: { + ssize_t sz = efidp_node_size(dp); + + if (SUB(sz, 4, &sz) || + sz < 0) { + efi_error("bad DP node size"); + return -1; + } format(buf, size, off, "Hardware", "HardwarePath(%d,", dp->subtype); - format_hex(buf, size, off, "Hardware", (uint8_t *)dp+4, - efidp_node_size(dp)-4); + format_hex(buf, size, off, "Hardware", (uint8_t *)dp+4, sz); format(buf, size, off, "Hardware", ")"); break; + } } return off; }