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