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

Fix build with gcc 8 and newer #1

Open
wants to merge 3 commits into
base: UDK2018
Choose a base branch
from

Conversation

NHellFire
Copy link

@NHellFire NHellFire commented Sep 29, 2020

gcc-8 (which is part of Fedora 28) enables the new warning
"-Wstringop-truncation" in "-Wall". This warning is documented in detail
at <https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html>; the
introduction says

> Warn for calls to bounded string manipulation functions such as strncat,
> strncpy, and stpncpy that may either truncate the copied string or leave
> the destination unchanged.

It breaks the BaseTools build with:

> EfiUtilityMsgs.c: In function 'PrintMessage':
> EfiUtilityMsgs.c:484:9: error: 'strncat' output may be truncated copying
> between 0 and 511 bytes from a string of length 511
> [-Werror=stringop-truncation]
>          strncat (Line, Line2, MAX_LINE_LEN - strlen (Line) - 1);
>          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> EfiUtilityMsgs.c:469:9: error: 'strncat' output may be truncated copying
> between 0 and 511 bytes from a string of length 511
> [-Werror=stringop-truncation]
>          strncat (Line, Line2, MAX_LINE_LEN - strlen (Line) - 1);
>          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> EfiUtilityMsgs.c:511:5: error: 'strncat' output may be truncated copying
> between 0 and 511 bytes from a string of length 511
> [-Werror=stringop-truncation]
>      strncat (Line, Line2, MAX_LINE_LEN - strlen (Line) - 1);
>      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The right way to fix the warning would be to implement string concat with
snprintf(). However, Microsoft does not appear to support snprintf()
before VS2015
<https://stackoverflow.com/questions/2915672/snprintf-and-visual-studio-2010>,
so we just have to shut up the warning. The strncat() calls flagged above
are valid BTW.

Cc: Ard Biesheuvel <[email protected]>
Cc: Cole Robinson <[email protected]>
Cc: Liming Gao <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Cc: Yonghong Zhu <[email protected]>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <[email protected]>
Reviewed-by: Liming Gao <[email protected]>
lersek and others added 2 commits September 30, 2020 19:07
…py()

gcc-8 (which is part of Fedora 28) enables the new warning
"-Wstringop-overflow" in "-Wall". This warning is documented in detail at
<https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html>; the
introduction says

> Warn for calls to string manipulation functions such as memcpy and
> strcpy that are determined to overflow the destination buffer.

It breaks the BaseTools build with:

> GenVtf.c: In function 'ConvertVersionInfo':
> GenVtf.c:132:7: error: 'strncpy' specified bound depends on the length
> of the source argument [-Werror=stringop-overflow=]
>        strncpy (TemStr + 4 - Length, Str, Length);
>        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> GenVtf.c:130:14: note: length computed here
>      Length = strlen(Str);
>               ^~~~~~~~~~~

It is a false positive because, while the bound equals the length of the
source argument, the destination pointer is moved back towards the
beginning of the destination buffer by the same amount (and this amount is
range-checked first, so we can't precede the start of the dest buffer).

Replace both strncpy() calls with memcpy().

Cc: Ard Biesheuvel <[email protected]>
Cc: Cole Robinson <[email protected]>
Cc: Liming Gao <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Cc: Yonghong Zhu <[email protected]>
Reported-by: Cole Robinson <[email protected]>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <[email protected]>
Reviewed-by: Liming Gao <[email protected]>
…FLAGS

GCC link script is used to discard the unused section data from ELF image.
ASLDLINK_FLAGS requires it to remove the unnecessary section data, then
GenFw can be used to retrieve the correct data section from ELF image.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Liming Gao <[email protected]>
Cc: Yonghong Zhu <[email protected]>
Reviewed-by: Yonghong Zhu <[email protected]>
Copy link

@JohnAZoidberg JohnAZoidberg left a comment

Choose a reason for hiding this comment

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

Can confirm that this works.

@jclab-joseph
Copy link

Is linuxboot no longer maintained?

hugelgupf pushed a commit that referenced this pull request Jan 24, 2024
Root cause:
1. Before DisableReadonlyPageWriteProtect() is called, the return
address (#1) is pushed in shadow stack.
2. CET is disabled.
3. DisableReadonlyPageWriteProtect() returns to #1.
4. Page table is modified.
5. EnableReadonlyPageWriteProtect() is called, but the return
address (#2) is not pushed in shadow stack.
6. CET is enabled.
7. EnableReadonlyPageWriteProtect() returns to #2.
#CP exception happens because the actual return address (#2)
doesn't match the return address stored in shadow stack (#1).

Analysis:
Shadow stack will stop update after CET disable (DisableCet() in
DisableReadOnlyPageWriteProtect), but normal smi stack will be
continue updated with the function called and return
(DisableReadOnlyPageWriteProtect & EnableReadOnlyPageWriteProtect),
thus leading stack mismatch after CET re-enabled (EnableCet() in
EnableReadOnlyPageWriteProtect).

According SDM Vol 3, 6.15-Control Protection Exception:
Normal smi stack and shadow stack must be matched when CET enable,
otherwise CP Exception will happen, which is caused by a near RET
instruction.

CET is disabled in DisableCet(), while can be enabled in
EnableCet(). This way won't cause the problem because they are
implemented in a way that return address of DisableCet() is
poped out from shadow stack (Incsspq performs a pop to increases
the shadow stack) and EnableCet() doesn't use "RET" but "JMP" to
return to caller. So calling EnableCet() and DisableCet() doesn't
have the same issue as calling DisableReadonlyPageWriteProtect()
and EnableReadonlyPageWriteProtect().

With above root cause & analysis, define below 2 macros instead of
functions for WP & CET operation:
WRITE_UNPROTECT_RO_PAGES (Wp, Cet)
WRITE_PROTECT_RO_PAGES (Wp, Cet)
Because DisableCet() & EnableCet() must be in the same function
to avoid shadow stack and normal SMI stack mismatch.

Note: WRITE_UNPROTECT_RO_PAGES () must be called pair with
WRITE_PROTECT_RO_PAGES () in same function.

Change-Id: I4e126697efcd8dbfb4887da034d8691bfca969e3
Cc: Eric Dong <[email protected]>
Cc: Ray Ni <[email protected]>
Cc: Zeng Star <[email protected]>
Cc: Gerd Hoffmann <[email protected]>
Cc: Rahul Kumar <[email protected]>
Cc: Laszlo Ersek <[email protected]>
Signed-off-by: Jiaxin Wu <[email protected]>
Reviewed-by: Laszlo Ersek <[email protected]>
Reviewed-by: Ray Ni <[email protected]>
Reviewed-by: Eric Dong <[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.

5 participants