[Linaro-uefi] Platforms/AMD/Styx: map the DXE stack as non-executable

Message ID 1481053337-13319-1-git-send-email-ard.biesheuvel@linaro.org
State Accepted
Commit 30a70640879494d0b975d37c2ceaf10e17f2a32c
Headers show

Commit Message

Ard Biesheuvel Dec. 6, 2016, 7:42 p.m.
Map the DXE stack as non-executable, to prevent stack buffer overflows
from being exploitable.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc                 | 3 +++
 Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc | 3 +++
 Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc         | 3 +++
 3 files changed, 9 insertions(+)

Comments

Leif Lindholm Dec. 7, 2016, 8:54 a.m. | #1
On Tue, Dec 06, 2016 at 07:42:17PM +0000, Ard Biesheuvel wrote:
> Map the DXE stack as non-executable, to prevent stack buffer overflows
> from being exploitable.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Any particular reason you're only doing this for the Styx platforms?

Anyway:
Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc                 | 3 +++
>  Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc | 3 +++
>  Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc         | 3 +++
>  3 files changed, 9 insertions(+)
> 
> diff --git a/Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc b/Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc
> index f833fe200422..0f299c388d00 100644
> --- a/Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc
> +++ b/Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc
> @@ -439,6 +439,9 @@ DEFINE DO_KCS    = 0
>    gAmdModulePkgTokenSpaceGuid.PcdSataSerdesBase|0xE1200000
>    gAmdModulePkgTokenSpaceGuid.PcdSataSerdesOffset|0x00010000
>  
> +  # map the stack as non-executable when entering the DXE phase
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE
> +
>  [PcdsPatchableInModule]
>  # PCIe Configuration: x4x2x2
>    gAmdModulePkgTokenSpaceGuid.PcdPcieCoreConfiguration|2
> diff --git a/Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc b/Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc
> index 107205386c55..0d630fba1ca9 100644
> --- a/Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc
> +++ b/Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc
> @@ -461,6 +461,9 @@ DEFINE DO_KCS       = 1
>    gAmdModulePkgTokenSpaceGuid.PcdSataSerdesBase|0xE1200000
>    gAmdModulePkgTokenSpaceGuid.PcdSataSerdesOffset|0x00010000
>  
> +  # map the stack as non-executable when entering the DXE phase
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE
> +
>  [PcdsPatchableInModule]
>  # PCIe Configuration: x4x2x2 (=2 See Include/FDKGionb.h)
>    gAmdModulePkgTokenSpaceGuid.PcdPcieCoreConfiguration|2
> diff --git a/Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc b/Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc
> index 92721064a51f..944cee3d8536 100644
> --- a/Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc
> +++ b/Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc
> @@ -458,6 +458,9 @@ DEFINE DO_KCS       = 1
>    gAmdModulePkgTokenSpaceGuid.PcdSataSerdesBase|0xE1200000
>    gAmdModulePkgTokenSpaceGuid.PcdSataSerdesOffset|0x00010000
>  
> +  # map the stack as non-executable when entering the DXE phase
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE
> +
>  !if $(DO_XGBE)
>    gAmdModulePkgTokenSpaceGuid.PcdXgbeEnable|TRUE
>  
> -- 
> 2.7.4
>
Ard Biesheuvel Dec. 7, 2016, 9 a.m. | #2
On 7 December 2016 at 08:54, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Tue, Dec 06, 2016 at 07:42:17PM +0000, Ard Biesheuvel wrote:
>> Map the DXE stack as non-executable, to prevent stack buffer overflows
>> from being exploitable.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> Any particular reason you're only doing this for the Styx platforms?
>

Those are the only ones I can test

> Anyway:
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
>

Thanks

>> ---
>>  Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc                 | 3 +++
>>  Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc | 3 +++
>>  Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc         | 3 +++
>>  3 files changed, 9 insertions(+)
>>
>> diff --git a/Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc b/Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc
>> index f833fe200422..0f299c388d00 100644
>> --- a/Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc
>> +++ b/Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc
>> @@ -439,6 +439,9 @@ DEFINE DO_KCS    = 0
>>    gAmdModulePkgTokenSpaceGuid.PcdSataSerdesBase|0xE1200000
>>    gAmdModulePkgTokenSpaceGuid.PcdSataSerdesOffset|0x00010000
>>
>> +  # map the stack as non-executable when entering the DXE phase
>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE
>> +
>>  [PcdsPatchableInModule]
>>  # PCIe Configuration: x4x2x2
>>    gAmdModulePkgTokenSpaceGuid.PcdPcieCoreConfiguration|2
>> diff --git a/Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc b/Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc
>> index 107205386c55..0d630fba1ca9 100644
>> --- a/Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc
>> +++ b/Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc
>> @@ -461,6 +461,9 @@ DEFINE DO_KCS       = 1
>>    gAmdModulePkgTokenSpaceGuid.PcdSataSerdesBase|0xE1200000
>>    gAmdModulePkgTokenSpaceGuid.PcdSataSerdesOffset|0x00010000
>>
>> +  # map the stack as non-executable when entering the DXE phase
>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE
>> +
>>  [PcdsPatchableInModule]
>>  # PCIe Configuration: x4x2x2 (=2 See Include/FDKGionb.h)
>>    gAmdModulePkgTokenSpaceGuid.PcdPcieCoreConfiguration|2
>> diff --git a/Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc b/Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc
>> index 92721064a51f..944cee3d8536 100644
>> --- a/Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc
>> +++ b/Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc
>> @@ -458,6 +458,9 @@ DEFINE DO_KCS       = 1
>>    gAmdModulePkgTokenSpaceGuid.PcdSataSerdesBase|0xE1200000
>>    gAmdModulePkgTokenSpaceGuid.PcdSataSerdesOffset|0x00010000
>>
>> +  # map the stack as non-executable when entering the DXE phase
>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE
>> +
>>  !if $(DO_XGBE)
>>    gAmdModulePkgTokenSpaceGuid.PcdXgbeEnable|TRUE
>>
>> --
>> 2.7.4
>>
Ard Biesheuvel Dec. 8, 2016, 9:30 a.m. | #3
On 7 December 2016 at 09:00, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 7 December 2016 at 08:54, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> On Tue, Dec 06, 2016 at 07:42:17PM +0000, Ard Biesheuvel wrote:
>>> Map the DXE stack as non-executable, to prevent stack buffer overflows
>>> from being exploitable.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>
>> Any particular reason you're only doing this for the Styx platforms?
>>
>
> Those are the only ones I can test
>

To elaborate: mapping the stack as executable involves the MMU page
table splitting code, which could trigger subtle issues involving TLB
conflicts.
Of course, we'd like to know about those asap, but blindly enabling it
for all platforms seems risky.
Leif Lindholm Dec. 8, 2016, 4:23 p.m. | #4
On Thu, Dec 08, 2016 at 09:30:26AM +0000, Ard Biesheuvel wrote:
> >>> Map the DXE stack as non-executable, to prevent stack buffer overflows
> >>> from being exploitable.
> >>>
> >>> Contributed-under: TianoCore Contribution Agreement 1.0
> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>
> >> Any particular reason you're only doing this for the Styx platforms?
> >
> > Those are the only ones I can test
> 
> To elaborate: mapping the stack as executable involves the MMU page
> table splitting code, which could trigger subtle issues involving TLB
> conflicts.
> Of course, we'd like to know about those asap, but blindly enabling it
> for all platforms seems risky.

Sure - but I guess something we would like to progressively migrate
towards?

/
    Leif
Ard Biesheuvel Dec. 8, 2016, 4:38 p.m. | #5
On 8 December 2016 at 16:23, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Thu, Dec 08, 2016 at 09:30:26AM +0000, Ard Biesheuvel wrote:
>> >>> Map the DXE stack as non-executable, to prevent stack buffer overflows
>> >>> from being exploitable.
>> >>>
>> >>> Contributed-under: TianoCore Contribution Agreement 1.0
>> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >>
>> >> Any particular reason you're only doing this for the Styx platforms?
>> >
>> > Those are the only ones I can test
>>
>> To elaborate: mapping the stack as executable involves the MMU page
>> table splitting code, which could trigger subtle issues involving TLB
>> conflicts.
>> Of course, we'd like to know about those asap, but blindly enabling it
>> for all platforms seems risky.
>
> Sure - but I guess something we would like to progressively migrate
> towards?
>

Yes, anything that improves robustness should be enabled where we can,
especially given how pathetic EDK2/Tianocore/UEFI are in this respect.

Patch

diff --git a/Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc b/Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc
index f833fe200422..0f299c388d00 100644
--- a/Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc
+++ b/Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc
@@ -439,6 +439,9 @@  DEFINE DO_KCS    = 0
   gAmdModulePkgTokenSpaceGuid.PcdSataSerdesBase|0xE1200000
   gAmdModulePkgTokenSpaceGuid.PcdSataSerdesOffset|0x00010000
 
+  # map the stack as non-executable when entering the DXE phase
+  gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE
+
 [PcdsPatchableInModule]
 # PCIe Configuration: x4x2x2
   gAmdModulePkgTokenSpaceGuid.PcdPcieCoreConfiguration|2
diff --git a/Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc b/Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc
index 107205386c55..0d630fba1ca9 100644
--- a/Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc
+++ b/Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc
@@ -461,6 +461,9 @@  DEFINE DO_KCS       = 1
   gAmdModulePkgTokenSpaceGuid.PcdSataSerdesBase|0xE1200000
   gAmdModulePkgTokenSpaceGuid.PcdSataSerdesOffset|0x00010000
 
+  # map the stack as non-executable when entering the DXE phase
+  gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE
+
 [PcdsPatchableInModule]
 # PCIe Configuration: x4x2x2 (=2 See Include/FDKGionb.h)
   gAmdModulePkgTokenSpaceGuid.PcdPcieCoreConfiguration|2
diff --git a/Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc b/Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc
index 92721064a51f..944cee3d8536 100644
--- a/Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc
+++ b/Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc
@@ -458,6 +458,9 @@  DEFINE DO_KCS       = 1
   gAmdModulePkgTokenSpaceGuid.PcdSataSerdesBase|0xE1200000
   gAmdModulePkgTokenSpaceGuid.PcdSataSerdesOffset|0x00010000
 
+  # map the stack as non-executable when entering the DXE phase
+  gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE
+
 !if $(DO_XGBE)
   gAmdModulePkgTokenSpaceGuid.PcdXgbeEnable|TRUE