mbox series

[edk2,RFC,0/4] RFC: increased memory protection

Message ID 1487787898-5222-1-git-send-email-ard.biesheuvel@linaro.org
Headers show
Series RFC: increased memory protection | expand

Message

Ard Biesheuvel Feb. 22, 2017, 6:24 p.m. UTC
Hello all,

This is a proof of concept implementation that removes all executable
permissions from writable memory regions, which greatly enhances security.
It is based on Jiewen's recent work, which is a step in the right direction,
but still leaves most of memory exploitable due to the default R+W+X
permissions.

The idea is that the implementation of the CPU arch protocol goes over the
memory map and removes exec permissions from all regions that are not already
marked as 'code. This requires some preparatory work to ensure that the DxeCore
itself is covered by a BootServicesCode region, not a BootServicesData region.
Exec permissions are re-granted selectively, when the PE/COFF loader allocates
the space for it. Combined with Jiewen's code/data split, this removes all
RWX mapped regions.

There is a caveat, though (and there are likely more of that kind): the EBC
driver will need some work to ensure the thunk buffers have the noexec
restriction lifted. This could be done in the EBC driver, but perhaps it is
better to either
a) modify the DXE core so it always removes noexec restrictions when allocating
   code pages, or
b) add AllocateExecPages/AllocateExecPool() functions to the MemoryAllocationLib
   API

Comments please!

Ard Biesheuvel (4):
  MdeModulePkg/DxeCore: allow BootServicesData->BootServicesCode
    conversion
  MdeModulePkg/DxeCore: convert the DxeCore memory region to
    BootServicesCode
  MdeModulePkg/DxeCore: lift non-exec permissions on loaded images
  ArmPkg/CpuDxe: remap all data regions non-executable

 ArmPkg/Drivers/CpuDxe/CpuDxe.c          | 76 ++++++++++++++++++++
 MdeModulePkg/Core/Dxe/DxeMain.h         |  8 +++
 MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c |  2 +
 MdeModulePkg/Core/Dxe/Image/Image.c     |  8 +++
 MdeModulePkg/Core/Dxe/Mem/Page.c        | 18 ++++-
 5 files changed, 111 insertions(+), 1 deletion(-)

-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Yao, Jiewen Feb. 23, 2017, 8:52 a.m. UTC | #1
HI Ard
Thanks to protect more. :-)
We did consider the idea to remove EXEC attribute for Data page before. But we got compatibility issue.

We documented some gaps in white paper -
https://github.com/tianocore-docs/Docs/raw/master/White_Papers/A_Tour_Beyond_BIOS_Memory_Map_And_Practices_in_UEFI_BIOS_V2.pdf

I am glad that some limitation is already resolved or we have solution to mitigate it. But there is still some other need consideration.


1)      We observe some 3rd part code allocating data page for code. - That is hardest part. (OS limitation #12)
We might also need consider ReservedMemory/AcpiNvs. There might be code there, too. (Firmware limitation #6 and #7).

If we want to apply the protection, we might need define a new PCD to disable the data protection for compatibility consideration.


2)      About DxeCore in data page. (Firmware limitation #4)
I am thinking if we can fix LoadFile implementation in PeiCore.

MdeModulePkg\Core\Pei\Image\Image.c:
LoadAndRelocatePeCoffImage()
{
ImageContext.ImageAddress = (EFI_PHYSICAL_ADDRESS)(UINTN) AllocatePages (EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize));
}

AllocatePages means to allocate data page.
I think we should use PeiAllocatePages(EfiBootServicesCode, EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize), &ImageContext.ImageAddress);

Does that fix the problem?



3)      I am not worried about EBC. That can be handled separately.


4)      I did not find patch 4/4 in my mail box. Maybe it is lost due to some unknown reason. Would you please send it again?


Thank you
Yao Jiewen


> -----Original Message-----

> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

> Sent: Thursday, February 23, 2017 2:25 AM

> To: edk2-devel@lists.01.org; afish@apple.com; leif.lindholm@linaro.org; Kinney,

> Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>;

> Yao, Jiewen <jiewen.yao@intel.com>

> Cc: lersek@redhat.com; Tian, Feng <feng.tian@intel.com>; Zeng, Star

> <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>

> Subject: [RFC PATCH 0/4] RFC: increased memory protection

>

> Hello all,

>

> This is a proof of concept implementation that removes all executable

> permissions from writable memory regions, which greatly enhances security.

> It is based on Jiewen's recent work, which is a step in the right direction,

> but still leaves most of memory exploitable due to the default R+W+X

> permissions.

>

> The idea is that the implementation of the CPU arch protocol goes over the

> memory map and removes exec permissions from all regions that are not already

> marked as 'code. This requires some preparatory work to ensure that the

> DxeCore

> itself is covered by a BootServicesCode region, not a BootServicesData region.

> Exec permissions are re-granted selectively, when the PE/COFF loader allocates

> the space for it. Combined with Jiewen's code/data split, this removes all

> RWX mapped regions.

>

> There is a caveat, though (and there are likely more of that kind): the EBC

> driver will need some work to ensure the thunk buffers have the noexec

> restriction lifted. This could be done in the EBC driver, but perhaps it is

> better to either

> a) modify the DXE core so it always removes noexec restrictions when allocating

>    code pages, or

> b) add AllocateExecPages/AllocateExecPool() functions to the

> MemoryAllocationLib

>    API

>

> Comments please!

>

> Ard Biesheuvel (4):

>   MdeModulePkg/DxeCore: allow BootServicesData->BootServicesCode

>     conversion

>   MdeModulePkg/DxeCore: convert the DxeCore memory region to

>     BootServicesCode

>   MdeModulePkg/DxeCore: lift non-exec permissions on loaded images

>   ArmPkg/CpuDxe: remap all data regions non-executable

>

>  ArmPkg/Drivers/CpuDxe/CpuDxe.c          | 76 ++++++++++++++++++++

>  MdeModulePkg/Core/Dxe/DxeMain.h         |  8 +++

>  MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c |  2 +

>  MdeModulePkg/Core/Dxe/Image/Image.c     |  8 +++

>  MdeModulePkg/Core/Dxe/Mem/Page.c        | 18 ++++-

>  5 files changed, 111 insertions(+), 1 deletion(-)

>

> --

> 2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek Feb. 23, 2017, 10:34 a.m. UTC | #2
Hi,

On 02/22/17 19:24, Ard Biesheuvel wrote:
> Hello all,

> 

> This is a proof of concept implementation that removes all executable

> permissions from writable memory regions, which greatly enhances security.

> It is based on Jiewen's recent work, which is a step in the right direction,

> but still leaves most of memory exploitable due to the default R+W+X

> permissions.

> 

> The idea is that the implementation of the CPU arch protocol goes over the

> memory map and removes exec permissions from all regions that are not already

> marked as 'code. This requires some preparatory work to ensure that the DxeCore

> itself is covered by a BootServicesCode region, not a BootServicesData region.

> Exec permissions are re-granted selectively, when the PE/COFF loader allocates

> the space for it. Combined with Jiewen's code/data split, this removes all

> RWX mapped regions.

> 

> There is a caveat, though (and there are likely more of that kind): the EBC

> driver will need some work to ensure the thunk buffers have the noexec

> restriction lifted. This could be done in the EBC driver, but perhaps it is

> better to either

> a) modify the DXE core so it always removes noexec restrictions when allocating

>    code pages, or

> b) add AllocateExecPages/AllocateExecPool() functions to the MemoryAllocationLib

>    API

> 

> Comments please!


My only comment is "good idea, as long as it doesn't regress anything". :)

I haven't done any testing with this; once you think the series is
mature enough for that, I could run my usual tests. Feel free to ping me
for that whenever.

Thanks!
Laszlo

> Ard Biesheuvel (4):

>   MdeModulePkg/DxeCore: allow BootServicesData->BootServicesCode

>     conversion

>   MdeModulePkg/DxeCore: convert the DxeCore memory region to

>     BootServicesCode

>   MdeModulePkg/DxeCore: lift non-exec permissions on loaded images

>   ArmPkg/CpuDxe: remap all data regions non-executable

> 

>  ArmPkg/Drivers/CpuDxe/CpuDxe.c          | 76 ++++++++++++++++++++

>  MdeModulePkg/Core/Dxe/DxeMain.h         |  8 +++

>  MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c |  2 +

>  MdeModulePkg/Core/Dxe/Image/Image.c     |  8 +++

>  MdeModulePkg/Core/Dxe/Mem/Page.c        | 18 ++++-

>  5 files changed, 111 insertions(+), 1 deletion(-)

> 


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Yao, Jiewen Feb. 23, 2017, 11:32 a.m. UTC | #3
One thing I am not clear is that why we choose Cpu driver to set the attribute, instead of DxeCore.

I think DxeCore might be a better place to perform such protection.

Thank you
Yao Jiewen


From: Yao, Jiewen

Sent: Thursday, February 23, 2017 4:52 PM
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-devel@lists.01.org; afish@apple.com; leif.lindholm@linaro.org; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>
Cc: lersek@redhat.com; Tian, Feng <feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
Subject: RE: [RFC PATCH 0/4] RFC: increased memory protection

HI Ard
Thanks to protect more. :-)
We did consider the idea to remove EXEC attribute for Data page before. But we got compatibility issue.

We documented some gaps in white paper -
https://github.com/tianocore-docs/Docs/raw/master/White_Papers/A_Tour_Beyond_BIOS_Memory_Map_And_Practices_in_UEFI_BIOS_V2.pdf

I am glad that some limitation is already resolved or we have solution to mitigate it. But there is still some other need consideration.


1)      We observe some 3rd part code allocating data page for code. - That is hardest part. (OS limitation #12)
We might also need consider ReservedMemory/AcpiNvs. There might be code there, too. (Firmware limitation #6 and #7).

If we want to apply the protection, we might need define a new PCD to disable the data protection for compatibility consideration.


2)      About DxeCore in data page. (Firmware limitation #4)
I am thinking if we can fix LoadFile implementation in PeiCore.

MdeModulePkg\Core\Pei\Image\Image.c:
LoadAndRelocatePeCoffImage()
{
ImageContext.ImageAddress = (EFI_PHYSICAL_ADDRESS)(UINTN) AllocatePages (EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize));
}

AllocatePages means to allocate data page.
I think we should use PeiAllocatePages(EfiBootServicesCode, EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize), &ImageContext.ImageAddress);

Does that fix the problem?



3)      I am not worried about EBC. That can be handled separately.


4)      I did not find patch 4/4 in my mail box. Maybe it is lost due to some unknown reason. Would you please send it again?


Thank you
Yao Jiewen


> -----Original Message-----

> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

> Sent: Thursday, February 23, 2017 2:25 AM

> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; afish@apple.com<mailto:afish@apple.com>; leif.lindholm@linaro.org<mailto:leif.lindholm@linaro.org>; Kinney,

> Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>>;

> Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>

> Cc: lersek@redhat.com<mailto:lersek@redhat.com>; Tian, Feng <feng.tian@intel.com<mailto:feng.tian@intel.com>>; Zeng, Star

> <star.zeng@intel.com<mailto:star.zeng@intel.com>>; Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>>

> Subject: [RFC PATCH 0/4] RFC: increased memory protection

>

> Hello all,

>

> This is a proof of concept implementation that removes all executable

> permissions from writable memory regions, which greatly enhances security.

> It is based on Jiewen's recent work, which is a step in the right direction,

> but still leaves most of memory exploitable due to the default R+W+X

> permissions.

>

> The idea is that the implementation of the CPU arch protocol goes over the

> memory map and removes exec permissions from all regions that are not already

> marked as 'code. This requires some preparatory work to ensure that the

> DxeCore

> itself is covered by a BootServicesCode region, not a BootServicesData region.

> Exec permissions are re-granted selectively, when the PE/COFF loader allocates

> the space for it. Combined with Jiewen's code/data split, this removes all

> RWX mapped regions.

>

> There is a caveat, though (and there are likely more of that kind): the EBC

> driver will need some work to ensure the thunk buffers have the noexec

> restriction lifted. This could be done in the EBC driver, but perhaps it is

> better to either

> a) modify the DXE core so it always removes noexec restrictions when allocating

>    code pages, or

> b) add AllocateExecPages/AllocateExecPool() functions to the

> MemoryAllocationLib

>    API

>

> Comments please!

>

> Ard Biesheuvel (4):

>   MdeModulePkg/DxeCore: allow BootServicesData->BootServicesCode

>     conversion

>   MdeModulePkg/DxeCore: convert the DxeCore memory region to

>     BootServicesCode

>   MdeModulePkg/DxeCore: lift non-exec permissions on loaded images

>   ArmPkg/CpuDxe: remap all data regions non-executable

>

>  ArmPkg/Drivers/CpuDxe/CpuDxe.c          | 76 ++++++++++++++++++++

>  MdeModulePkg/Core/Dxe/DxeMain.h         |  8 +++

>  MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c |  2 +

>  MdeModulePkg/Core/Dxe/Image/Image.c     |  8 +++

>  MdeModulePkg/Core/Dxe/Mem/Page.c        | 18 ++++-

>  5 files changed, 111 insertions(+), 1 deletion(-)

>

> --

> 2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Feb. 23, 2017, 11:39 a.m. UTC | #4
On 23 February 2017 at 08:52, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> HI Ard
>
> Thanks to protect more. :-)
>

Of course! This is a very important topic for me.

> We did consider the idea to remove EXEC attribute for Data page before. But
> we got compatibility issue.
>
>
>
> We documented some gaps in white paper -
>
> https://github.com/tianocore-docs/Docs/raw/master/White_Papers/A_Tour_Beyond_BIOS_Memory_Map_And_Practices_in_UEFI_BIOS_V2.pdf
>

Thanks for the link.

> I am glad that some limitation is already resolved or we have solution to
> mitigate it. But there is still some other need consideration.
>
>
>
> 1)      We observe some 3rd part code allocating data page for code. – That
> is hardest part. (OS limitation #12)
>
> We might also need consider ReservedMemory/AcpiNvs. There might be code
> there, too. (Firmware limitation #6 and #7).
>

OK

> If we want to apply the protection, we might need define a new PCD to
> disable the data protection for compatibility consideration.
>
>
>
> 2)      About DxeCore in data page. (Firmware limitation #4)
>
> I am thinking if we can fix LoadFile implementation in PeiCore.
>
>
>
> MdeModulePkg\Core\Pei\Image\Image.c:
>
> LoadAndRelocatePeCoffImage()
>
> {
>
> ImageContext.ImageAddress = (EFI_PHYSICAL_ADDRESS)(UINTN) AllocatePages
> (EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize));
>
> }
>
>
>
> AllocatePages means to allocate data page.
>
> I think we should use PeiAllocatePages(EfiBootServicesCode,
> EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize), &ImageContext.ImageAddress);
>
>
>
> Does that fix the problem?
>

Using PeiServicesAllocatePage() in the way that you describe does
indeed remove the problem, so I will use that instead.

> 3)      I am not worried about EBC. That can be handled separately.
>

OK


> 4)      I did not find patch 4/4 in my mail box. Maybe it is lost due to
> some unknown reason. Would you please send it again?
>

https://lists.01.org/pipermail/edk2-devel/2017-February/007685.html

I will send out a v2 shortly which, as you suggest, moves the handling
to DXE core. The only problem is that ARM's SyncCacheConfig() removes
the noexec attributes again, so I need to fix that first. Then, the
arch CPU protocol installation event can iterate over the memory map
to set the permissions according to a policy PCD

Thanks,
Ard.
Yao, Jiewen Feb. 23, 2017, 11:45 a.m. UTC | #5
Sounds great.
I look forward to your V2.

Thank you
Yao Jiewen

From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

Sent: Thursday, February 23, 2017 7:39 PM
To: Yao, Jiewen <jiewen.yao@intel.com>
Cc: edk2-devel@lists.01.org; afish@apple.com; leif.lindholm@linaro.org; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; lersek@redhat.com; Tian, Feng <feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: Re: [RFC PATCH 0/4] RFC: increased memory protection

On 23 February 2017 at 08:52, Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> wrote:
> HI Ard

>

> Thanks to protect more. :-)

>


Of course! This is a very important topic for me.

> We did consider the idea to remove EXEC attribute for Data page before. But

> we got compatibility issue.

>

>

>

> We documented some gaps in white paper -

>

> https://github.com/tianocore-docs/Docs/raw/master/White_Papers/A_Tour_Beyond_BIOS_Memory_Map_And_Practices_in_UEFI_BIOS_V2.pdf

>


Thanks for the link.

> I am glad that some limitation is already resolved or we have solution to

> mitigate it. But there is still some other need consideration.

>

>

>

> 1)      We observe some 3rd part code allocating data page for code. – That

> is hardest part. (OS limitation #12)

>

> We might also need consider ReservedMemory/AcpiNvs. There might be code

> there, too. (Firmware limitation #6 and #7).

>


OK

> If we want to apply the protection, we might need define a new PCD to

> disable the data protection for compatibility consideration.

>

>

>

> 2)      About DxeCore in data page. (Firmware limitation #4)

>

> I am thinking if we can fix LoadFile implementation in PeiCore.

>

>

>

> MdeModulePkg\Core\Pei\Image\Image.c:

>

> LoadAndRelocatePeCoffImage()

>

> {

>

> ImageContext.ImageAddress = (EFI_PHYSICAL_ADDRESS)(UINTN) AllocatePages

> (EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize));

>

> }

>

>

>

> AllocatePages means to allocate data page.

>

> I think we should use PeiAllocatePages(EfiBootServicesCode,

> EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize), &ImageContext.ImageAddress);

>

>

>

> Does that fix the problem?

>


Using PeiServicesAllocatePage() in the way that you describe does
indeed remove the problem, so I will use that instead.

> 3)      I am not worried about EBC. That can be handled separately.

>


OK


> 4)      I did not find patch 4/4 in my mail box. Maybe it is lost due to

> some unknown reason. Would you please send it again?

>


https://lists.01.org/pipermail/edk2-devel/2017-February/007685.html

I will send out a v2 shortly which, as you suggest, moves the handling
to DXE core. The only problem is that ARM's SyncCacheConfig() removes
the noexec attributes again, so I need to fix that first. Then, the
arch CPU protocol installation event can iterate over the memory map
to set the permissions according to a policy PCD

Thanks,
Ard.
Ard Biesheuvel Feb. 23, 2017, 7:32 p.m. UTC | #6
On 23 February 2017 at 11:45, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> Sounds great.

>

> I look forward to your V2.

>


Hello Jiewen,

What I am currently struggling with is the fact that we don't use the
GCD RO/XP permissions at all. This means that
RefreshGcdMemoryAttributes () (or SyncCacheConfig() on ARM) will
remove non-exec attributes if we add them in the CPU arch protocol
installation notifier callback.

So there are two approaches imo:
- introduce a way to call into the DXE core to mark all non-code
regions non-exec after RefreshGcdMemoryAttributes () has been called,
or
- add the RO/XP attributes to the GCD memory space map, and enable
them in the attributes.

Option #2 will require a change to CoreAddRange to prevent those RO/XP
attributes to leak into the UEFI memory map, because that results in
all regions have to RO/XP attributes set by default, which is
obviously not what we want.

Any thoughts?

Thanks,
Ard.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Yao, Jiewen Feb. 24, 2017, 2:25 a.m. UTC | #7
HI Ard
In X86 CPU driver - UefiCpuPkg\CpuDxe, we use a global variable – mIsFlushingGCD.

In RefreshGcdMemoryAttributes(), we set mIsFlushingGCD=TRUE.
In CpuSetMemoryAttributes(), we check mIsFlushingGCD. If mIsFlushingGCD is TRUE, CpuSetMemoryAttributes() returns immediately without touching cache attribute or memory attribute.

The reason is that RefreshGcdMemoryAttributes() just sync current CPU hardware setting to GCD software record.
No real need to set cache again.

Previous we purposely skip GCD setting on RO/XP, the reason is still compatibility concern.
We do not want to provide a different memory map to 3rd part code, just in case there is hidden assumption on memory map attributes.


Maybe ARM can use similar way in SyncCacheConfig() and do a simple check in CpuSetMemoryAttributes().

Thank you
Yao Jiewen

From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

Sent: Friday, February 24, 2017 3:33 AM
To: Yao, Jiewen <jiewen.yao@intel.com>
Cc: edk2-devel@lists.01.org; afish@apple.com; leif.lindholm@linaro.org; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; lersek@redhat.com; Tian, Feng <feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: Re: [RFC PATCH 0/4] RFC: increased memory protection

On 23 February 2017 at 11:45, Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> wrote:
> Sounds great.

>

> I look forward to your V2.

>


Hello Jiewen,

What I am currently struggling with is the fact that we don't use the
GCD RO/XP permissions at all. This means that
RefreshGcdMemoryAttributes () (or SyncCacheConfig() on ARM) will
remove non-exec attributes if we add them in the CPU arch protocol
installation notifier callback.

So there are two approaches imo:
- introduce a way to call into the DXE core to mark all non-code
regions non-exec after RefreshGcdMemoryAttributes () has been called,
or
- add the RO/XP attributes to the GCD memory space map, and enable
them in the attributes.

Option #2 will require a change to CoreAddRange to prevent those RO/XP
attributes to leak into the UEFI memory map, because that results in
all regions have to RO/XP attributes set by default, which is
obviously not what we want.

Any thoughts?

Thanks,
Ard.