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.

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

Rename 'msecs' to 'usecs' to avoid potential confusion #563

Closed
wants to merge 5 commits into from

Conversation

aronowski
Copy link
Contributor

As suggested in #251.

The function msleep uses gBS->Stall which waits for a specified number of microseconds.

Reference: https://edk2-docs.gitbook.io/edk-ii-uefi-driver-writer-s-guide/5_uefi_services/51_services_that_uefi_drivers_commonly_use/517_stall

This reference even mentions an example slepping for 10 microseconds: // Wait 10 uS. Notice the letter 'u'.

Therefore it's a good idea to call the argument msleep uses 'usecs' rather than 'msecs' so no one confuses it with miliseconds.

The function msleep uses gBS->Stall which waits for a specified number of microseconds.

Reference: https://edk2-docs.gitbook.io/edk-ii-uefi-driver-writer-s-guide/5_uefi_services/51_services_that_uefi_drivers_commonly_use/517_stall

This reference even mentions an example slepping for 10 microseconds: // Wait 10 uS. Notice the letter 'u'.

Therefore it's a good idea to call the argument msleep uses 'usecs' rather than
'msecs' so no one confuses it with miliseconds.

Signed-off-by: Kamil Aronowski <[email protected]>
@dennis-tseng99
Copy link
Contributor

dennis-tseng99 commented Apr 24, 2023

Normally, parameter usecs is used for usleep(useconds_t usec), and msecs is used for msleep(unsigned int msecs). Hence, to avoid confusion, could we also change the function name from msleep() to usleep() ?

@aronowski
Copy link
Contributor Author

I like this idea but the implementation of it may be more complex than I initially thought.

I'm not a developer as of today and I'm unsure if such a change won't break anything crucial. I don't want to introduce major changes especially for a security-critical component such as Shim.

Changing argument names only resulted in the exact same binary, while changing function names results in a binary with a different checksum. While it works fine and allows for booting a system with Secure Boot enabled, I'm unsure if I should do this.

Maybe I'll let the Red Hat Bootloader Team decide on this.

@vathpela
Copy link
Contributor

vathpela commented May 2, 2023

The function name should be changed as well.

@dennis-tseng99
Copy link
Contributor

@aronowski If you don't mind, please let me change the function name and its argument as well in another PR according to vathpela's suggestion.

Normally, parameter usecs is used for usleep(useconds_t usec), and msecs is used for msleep(unsigned int msecs). This commit changes the function name from msleep() to usleep() to avoid futher confusion.

Signed-off-by: Kamil Aronowski <[email protected]>
@aronowski
Copy link
Contributor Author

The function has been renamed according to Peter's request.

@dennis-tseng99 I did this change once I saw my first email notification rather than following the complete conversation. You can, however, review the latest commit so I can be sure I haven't made any mistake.

Copy link
Contributor

@dennis-tseng99 dennis-tseng99 left a comment

Choose a reason for hiding this comment

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

In fallback.c, would you please also change int fallback_verbose_wait to unsigned long ?

1009                 - int fallback_verbose_wait = 500000; /* default to 0.5s */
1009                 + unsigned long fallback_verbose_wait = 500000; /* default to 0.5s */
1016                 usleep(fallback_verbose_wait);
1214                 - int fallback_verbose_wait = 500000; /* default to 0.5s */
1214                 + unsigned long fallback_verbose_wait = 500000; /* default to 0.5s */
1221                 usleep(fallback_verbose_wait);

@vathpela
Copy link
Contributor

vathpela commented May 5, 2023

I think wrapping the decl in a test for SHIM_UNIT_TEST should fix the unit test failures:

diff --git a/include/console.h b/include/console.h
index 04f02da7e01..8eb4b47f912 100644
--- a/include/console.h
+++ b/include/console.h
@@ -122,7 +122,9 @@ extern EFI_STATUS EFIAPI vdprint_(const CHAR16 *fmt, const char *file, int line,
 extern EFI_STATUS print_crypto_errors(EFI_STATUS rc, char *file, const char *func, int line);
 #define crypterr(rc) print_crypto_errors((rc), __FILE__, __func__, __LINE__)

+#ifndef SHIM_UNIT_TEST
 extern VOID usleep(unsigned long usecs);
+#endif

 /* This is used in various things to determine if we should print to the
  * console */

aronowski added 2 commits May 8, 2023 10:11
In some cases tests were failing due to a mismatch between our
declaration of the usleep() function and what is being provided by
unistd.h. This change simply makes our function declared only when not
in a unit test environment.

Signed-off-by: Kamil Aronowski <[email protected]>
The variable fallback_verbose_wait from now on is of the type unsigned
long to match the type of the argument usleep() accepts.

Signed-off-by: Kamil Aronowski <[email protected]>
@aronowski
Copy link
Contributor Author

@dennis-tseng99 @vathpela
Thanks a lot for the hints. I've added the two changes.
In both cases I've confirmed in my public fork that unit tests pass.

@aronowski aronowski requested a review from dennis-tseng99 May 8, 2023 08:30
Copy link
Contributor

@dennis-tseng99 dennis-tseng99 left a comment

Choose a reason for hiding this comment

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

Except header file, may I suggest we also add "#ifndef SHIM_UNIT_TEST" to the function itself ?

/* lib/console.c */
#ifndef SHIM_UNIT_TEST
VOID
usleep(unsigned long usecs)
{
	BS->Stall(usecs);
}
#endif

@aronowski
Copy link
Contributor Author

Sure thing. It's right there! 😀

@aronowski aronowski requested a review from dennis-tseng99 May 19, 2023 07:16
@vathpela
Copy link
Contributor

I've squashed this up and re-ordered it a bit so that each commit passes the tests, and pushed it as 549d346 and 908c388.

@vathpela vathpela closed this Jun 21, 2023
vathpela added a commit that referenced this pull request Jan 23, 2024
What's changed
* Various CVE fixes:
  CVE-2023-40546 mok: fix LogError() invocation
  CVE-2023-40547 - avoid incorrectly trusting HTTP headers
  CVE-2023-40548 Fix integer overflow on SBAT section size on 32-bit system
  CVE-2023-40549 Authenticode: verify that the signature header is in bounds.
  CVE-2023-40550 pe: Fix an out-of-bound read in verify_buffer_sbat()
  CVE-2023-40551: pe-relocate: Fix bounds check for MZ binaries
* Add make infrastructure to set the NX_COMPAT flag by @vathpela in #530
* Make sbat_var.S parse right with buggy gcc/binutils by @vathpela in #535
* Drop invalid calls to CRYPTO_set_mem_functions by @nicholasbishop in #537
* pe: Align section size up to page size for mem attrs by @nicholasbishop in #539
* test-sbat: Fix exit code by @vathpela in #540
* pe: Add IS_PAGE_ALIGNED macro by @nicholasbishop in #541
* CryptoPkg/BaseCryptLib: Fix buffer overflow issue in realloc wrapper by @nicholasbishop in #546
* Don't loop forever in load_certs() with buggy firmware by @rmetrich in #547
* Block Debian grub binaries with SBAT < 4 by @steve-mcintyre in #550
* Shim unable to locate grubx64 in PXE boot mode when grubx64 is stored in a different file path by @Alberto-Perez-Guevara in #551
* Further improve load_certs() for non-compliant drivers/firmwares by @pbatard in #560
* pe: only process RelocDir->Size of reloc section by @mikebeaton in #562
* Rename 'msecs' to 'usecs' to avoid potential confusion by @aronowski in #563
* Optionally allow to keep shim protocol installed by @bluca in #565
* SBAT-related documents formatting and spelling by @aronowski in #566
* Add SbatLevel_Variable.txt to document the various revocations by @jsetje in #569
* Add a security contact email address in README.md by @vathpela in #572
* Use -Wno-unused-but-set-variable for Cryptlib and OpenSSL by @vathpela in #576
* mok: fix LogError() invocation by @vathpela in #577
* Minor housekeeping by @vathpela in #578
* Test ImageAddress() by @vathpela in #579
* FreePages() is used to return memory allocated by AllocatePages() by @dennis-tseng99 in #580
* Size should minus 1 when calculating 'RelocBaseEnd' by @jsetje in #581
* Verify signature before verifying sbat levels by @jsetje in #583
* Add libFuzzer support for csv.c and sbat.c by @vathpela in #584
* mok: Avoid underflow in maximum variable size calculation by @alpernebbi in #587
* Housekeeping by @vathpela in #605

Signed-off-by: Peter Jones <[email protected]>
brianredbeard pushed a commit to brianredbeard/redhat-efi-boot-shim that referenced this pull request Feb 22, 2024
What's changed
* Various CVE fixes:
  CVE-2023-40546 mok: fix LogError() invocation
  CVE-2023-40547 - avoid incorrectly trusting HTTP headers
  CVE-2023-40548 Fix integer overflow on SBAT section size on 32-bit system
  CVE-2023-40549 Authenticode: verify that the signature header is in bounds.
  CVE-2023-40550 pe: Fix an out-of-bound read in verify_buffer_sbat()
  CVE-2023-40551: pe-relocate: Fix bounds check for MZ binaries
* Add make infrastructure to set the NX_COMPAT flag by @vathpela in rhboot#530
* Make sbat_var.S parse right with buggy gcc/binutils by @vathpela in rhboot#535
* Drop invalid calls to CRYPTO_set_mem_functions by @nicholasbishop in rhboot#537
* pe: Align section size up to page size for mem attrs by @nicholasbishop in rhboot#539
* test-sbat: Fix exit code by @vathpela in rhboot#540
* pe: Add IS_PAGE_ALIGNED macro by @nicholasbishop in rhboot#541
* CryptoPkg/BaseCryptLib: Fix buffer overflow issue in realloc wrapper by @nicholasbishop in rhboot#546
* Don't loop forever in load_certs() with buggy firmware by @rmetrich in rhboot#547
* Block Debian grub binaries with SBAT < 4 by @steve-mcintyre in rhboot#550
* Shim unable to locate grubx64 in PXE boot mode when grubx64 is stored in a different file path by @Alberto-Perez-Guevara in rhboot#551
* Further improve load_certs() for non-compliant drivers/firmwares by @pbatard in rhboot#560
* pe: only process RelocDir->Size of reloc section by @mikebeaton in rhboot#562
* Rename 'msecs' to 'usecs' to avoid potential confusion by @aronowski in rhboot#563
* Optionally allow to keep shim protocol installed by @bluca in rhboot#565
* SBAT-related documents formatting and spelling by @aronowski in rhboot#566
* Add SbatLevel_Variable.txt to document the various revocations by @jsetje in rhboot#569
* Add a security contact email address in README.md by @vathpela in rhboot#572
* Use -Wno-unused-but-set-variable for Cryptlib and OpenSSL by @vathpela in rhboot#576
* mok: fix LogError() invocation by @vathpela in rhboot#577
* Minor housekeeping by @vathpela in rhboot#578
* Test ImageAddress() by @vathpela in rhboot#579
* FreePages() is used to return memory allocated by AllocatePages() by @dennis-tseng99 in rhboot#580
* Size should minus 1 when calculating 'RelocBaseEnd' by @jsetje in rhboot#581
* Verify signature before verifying sbat levels by @jsetje in rhboot#583
* Add libFuzzer support for csv.c and sbat.c by @vathpela in rhboot#584
* mok: Avoid underflow in maximum variable size calculation by @alpernebbi in rhboot#587
* Housekeeping by @vathpela in rhboot#605

Signed-off-by: Peter Jones <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants