Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix incorrect error handling in callers of efidp_node_size() #264

Merged
merged 9 commits into from
Mar 6, 2024
Merged

Fix incorrect error handling in callers of efidp_node_size() #264

merged 9 commits into from
Mar 6, 2024

Conversation

vathpela
Copy link
Contributor

@vathpela vathpela commented Mar 6, 2024

One of our internal analyzers noticed some issues in our usage of efidp_node_size() in the device path formatters. This patch attempts to address those issues.

vathpela and others added 9 commits March 6, 2024 11:53
@vathpela
safemath: fix users of is_null()
ed17f9f 
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 <[email protected]>
@vathpela
Fix wrong variadic initialization
606faed 
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 <[email protected]>
@lsandov1 @vathpela
src/dp-media.c: fix overruns on efidp_node_size calls
8a24122 
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 <[email protected]>
@vathpela
dp-media: more efidp_node_size() fixups.
812f20b 
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 <[email protected]>
@vathpela
dp.h: Fix incorrect efidp_node_size() error handling
d0fa2b8 
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 <[email protected]>
@vathpela
efidp_format_device_path(): fix efidp_node_size() return value handling.
400bad7 
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 <[email protected]>
@vathpela
dp-message: fix efidp_node_size() error checking
57d1023 
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 <[email protected]>
@vathpela
dp-acpi.c: fix incorrect error handling of efidp_node_size() invocations
4e09015 
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 <[email protected]>
@vathpela
dp-hw.c: fix incorrect error handling of efidp_node_size() invocations
b580d3b 
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 <[email protected]>
lsandov1
lsandov1 reviewed Mar 6, 2024
View reviewed changes
Copy link
Contributor

@lsandov1 lsandov1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nfrayer
Copy link

nfrayer commented Mar 6, 2024

lgtm

@vathpela vathpela merged commit c71c434 into rhboot:main Mar 6, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lsandov1 lsandov1 lsandov1 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vathpela @nfrayer @lsandov1