Re: [edk2] [PATCH v2 0/2] Resolve feature conflict between NX and Stack guard
On 3/7/2018 12:28 PM, Wu, Hao A wrote: Hi Ray, Below are the answers to your feedbacks: -Original Message- From: Yao, Jiewen Sent: Wednesday, March 07, 2018 12:14 PM To: Wang, Jian J; Ni, Ruiyu; Wu, Hao A; edk2-devel@lists.01.org Cc: Zeng, Star; Dong, Eric Subject: RE: [PATCH v2 0/2] Resolve feature conflict between NX and Stack guard I think the original patch is fine. StackBase is already checked by using ASSERT before. +ASSERT ((StackBase & EFI_PAGE_MASK) == 0); MemMap entry must be page aligned. No additional check is required here. Thank you Yao Jiewen -Original Message- From: Wang, Jian J Sent: Wednesday, March 7, 2018 11:40 AM To: Ni, Ruiyu ; Wu, Hao A ; edk2-devel@lists.01.org Cc: Zeng, Star ; Dong, Eric ; Yao, Jiewen Subject: RE: [PATCH v2 0/2] Resolve feature conflict between NX and Stack guard Regards, Jian -Original Message- From: Ni, Ruiyu Sent: Wednesday, March 07, 2018 11:17 AM To: Wu, Hao A ; edk2-devel@lists.01.org Cc: Wang, Jian J ; Zeng, Star ; Dong, Eric ; Yao, Jiewen Subject: Re: [PATCH v2 0/2] Resolve feature conflict between NX and Stack guard On 3/6/2018 9:33 PM, Hao Wu wrote: V2 changes: A. Use Hoblib APIs to get the base of stack from Hob. B. Remove unnecessary local variable used in function InitializeDxeNxMemoryProtectionPolicy(). V1 history: If enabled, NX memory protection feature will mark some types of active memory as NX (non-executable), which includes the first page of the stack. This will overwrite the attributes of the first page of the stack if the stack guard feature is also enabled. The series will override the attributes setting to the first page of the stack by adding back the 'EFI_MEMORY_RP' attribute when the stack guard feature is enabled. Cc: Jian J Wang Cc: Star Zeng Cc: Eric Dong Cc: Jiewen Yao Cc: Ruiyu Ni Hao Wu (2): MdeModulePkg/Core: Refine handling NULL detection in NX setting MdeModulePkg/Core: Fix feature conflict between NX and Stack guard MdeModulePkg/Core/Dxe/DxeMain.inf | 4 +- MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 74 +++ 2 files changed, 67 insertions(+), 11 deletions(-) if (MemoryMapEntry->PhysicalStart == 0 && PcdGet8 (PcdNullPointerDetectionPropertyMask) != 0) { ASSERT (MemoryMapEntry->NumberOfPages > 0); // // Add EFI_MEMORY_RP attribute for page 0 if NULL pointer detection is // enabled. // [Ray] 1. I prefer to move the above comments before the "if (...)". Yes. I agree that moving the comments block out of the 'if' statement is more readable. I will update according to your suggestion when I pushing the changes. SetUefiImageMemoryAttributes ( 0, EFI_PAGES_TO_SIZE (1), EFI_MEMORY_RP | Attributes); } if (StackBase != 0 && (StackBase >= MemoryMapEntry->PhysicalStart && StackBase < MemoryMapEntry->PhysicalStart + LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT)) && PcdGetBool (PcdCpuStackGuard)) { // // Add EFI_MEMORY_RP attribute for the first page of the stack if stack // guard is enabled. // SetUefiImageMemoryAttributes ( StackBase, EFI_PAGES_TO_SIZE (1), EFI_MEMORY_RP | Attributes); [Ray] 2. The StackBase is directly used here. So do we need to check whether it's page aligned? Do we need to check whether the range [StackBase, StackBase + 4KB) is inside the MemoryMapEntry? } If PcdCpuStackGuard is TRUE, I think the owner who allocates memory for StackBase should make sure all the conditions you mentioned, but not here. Just as Jiewen mentioned in the previous reply. An ASSERT: ASSERT ((StackBase & EFI_PAGE_MASK) == 0); is added to ensure the stack base fetched from Hob is page-size aligned. And the below check: (StackBase >= MemoryMapEntry->PhysicalStart && StackBase < MemoryMapEntry->PhysicalStart + LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT)) together with the fact that MemMap is page aligned (also mentioned by Jiewen) ensures that the first page of the stack is cover by the memory range of the MemMap. I agree. Thanks for the explanation. Please move the comments location when checking in the patch. Best Regards, Hao Wu -- Thanks, Ray -- Thanks, Ray ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 0/2] Resolve feature conflict between NX and Stack guard
Hi Ray, Below are the answers to your feedbacks: > -Original Message- > From: Yao, Jiewen > Sent: Wednesday, March 07, 2018 12:14 PM > To: Wang, Jian J; Ni, Ruiyu; Wu, Hao A; edk2-devel@lists.01.org > Cc: Zeng, Star; Dong, Eric > Subject: RE: [PATCH v2 0/2] Resolve feature conflict between NX and Stack > guard > > I think the original patch is fine. > > StackBase is already checked by using ASSERT before. > > +ASSERT ((StackBase & EFI_PAGE_MASK) == 0); > > MemMap entry must be page aligned. > > No additional check is required here. > > Thank you > Yao Jiewen > > > > -Original Message- > > From: Wang, Jian J > > Sent: Wednesday, March 7, 2018 11:40 AM > > To: Ni, Ruiyu ; Wu, Hao A ; > > edk2-devel@lists.01.org > > Cc: Zeng, Star ; Dong, Eric ; Yao, > > Jiewen > > Subject: RE: [PATCH v2 0/2] Resolve feature conflict between NX and Stack > guard > > > > > > > > Regards, > > Jian > > > > > > > -Original Message- > > > From: Ni, Ruiyu > > > Sent: Wednesday, March 07, 2018 11:17 AM > > > To: Wu, Hao A ; edk2-devel@lists.01.org > > > Cc: Wang, Jian J ; Zeng, Star > ; > > > Dong, Eric ; Yao, Jiewen > > > Subject: Re: [PATCH v2 0/2] Resolve feature conflict between NX and Stack > > > guard > > > > > > On 3/6/2018 9:33 PM, Hao Wu wrote: > > > > V2 changes: > > > > > > > > A. Use Hoblib APIs to get the base of stack from Hob. > > > > B. Remove unnecessary local variable used in function > > > > InitializeDxeNxMemoryProtectionPolicy(). > > > > > > > > V1 history: > > > > > > > > If enabled, NX memory protection feature will mark some types of active > > > > memory as NX (non-executable), which includes the first page of the > > > > stack. > > > > This will overwrite the attributes of the first page of the stack if the > > > > stack guard feature is also enabled. > > > > > > > > The series will override the attributes setting to the first page of the > > > > stack by adding back the 'EFI_MEMORY_RP' attribute when the stack > guard > > > > feature is enabled. > > > > > > > > Cc: Jian J Wang > > > > Cc: Star Zeng > > > > Cc: Eric Dong > > > > Cc: Jiewen Yao > > > > Cc: Ruiyu Ni > > > > > > > > Hao Wu (2): > > > >MdeModulePkg/Core: Refine handling NULL detection in NX setting > > > >MdeModulePkg/Core: Fix feature conflict between NX and Stack guard > > > > > > > > MdeModulePkg/Core/Dxe/DxeMain.inf | 4 +- > > > > MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 74 > > > +++ > > > > 2 files changed, 67 insertions(+), 11 deletions(-) > > > > > > > > > >if (MemoryMapEntry->PhysicalStart == 0 && > > >PcdGet8 (PcdNullPointerDetectionPropertyMask) != 0) { > > > > > > ASSERT (MemoryMapEntry->NumberOfPages > 0); > > > // > > > // Add EFI_MEMORY_RP attribute for page 0 if NULL pointer > > > detection is > > > // enabled. > > > // > > > [Ray] 1. I prefer to move the above comments before the "if (...)". Yes. I agree that moving the comments block out of the 'if' statement is more readable. I will update according to your suggestion when I pushing the changes. > > > > > > SetUefiImageMemoryAttributes ( > > >0, > > >EFI_PAGES_TO_SIZE (1), > > >EFI_MEMORY_RP | Attributes); > > >} > > > > > >if (StackBase != 0 && > > >(StackBase >= MemoryMapEntry->PhysicalStart && > > > StackBase < MemoryMapEntry->PhysicalStart + > > > LShiftU64 > > (MemoryMapEntry->NumberOfPages, > > > EFI_PAGE_SHIFT)) && > > >PcdGetBool (PcdCpuStackGuard)) { > > > > > > // > > > // Add EFI_MEMORY_RP attribute for the first page of the stack > > > if stack > > > // guard is enabled. > > > // > > > SetUefiImageMemoryAttributes ( > > >StackBase, > > >EFI_PAGES_TO_SIZE (1), > > >EFI_MEMORY_RP | Attributes); > > > [Ray] 2. The StackBase is directly used here. So do we need to check > > > whether it's page aligned? Do we need to check whether the range > > > [StackBase, StackBase + 4KB) is inside the MemoryMapEntry? > > >} > > > > If PcdCpuStackGuard is TRUE, I think the owner who allocates memory for > > StackBase > > should make sure all the conditions you mentioned, but not here. > > Just as Jiewen mentioned in the previous reply. An ASSERT: ASSERT ((StackBase & EFI_PAGE_MASK) == 0); is added to ensure the stack base fetched from Hob is page-size aligned. And the below check: (StackBase >= MemoryMapEntry->PhysicalStart && StackBase < MemoryMapEntry->PhysicalStart + LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT)) together with the fact that MemMap is page aligned (also mentioned by Jiewen) ensures that the first page of the stack is cover by the memory range of the MemMap. Best Regards, Hao Wu > > > > > > -- > > > Thanks, > > > Ray _
Re: [edk2] [PATCH v2 0/2] Resolve feature conflict between NX and Stack guard
I think the original patch is fine. StackBase is already checked by using ASSERT before. > +ASSERT ((StackBase & EFI_PAGE_MASK) == 0); MemMap entry must be page aligned. No additional check is required here. Thank you Yao Jiewen > -Original Message- > From: Wang, Jian J > Sent: Wednesday, March 7, 2018 11:40 AM > To: Ni, Ruiyu ; Wu, Hao A ; > edk2-devel@lists.01.org > Cc: Zeng, Star ; Dong, Eric ; Yao, > Jiewen > Subject: RE: [PATCH v2 0/2] Resolve feature conflict between NX and Stack > guard > > > > Regards, > Jian > > > > -Original Message- > > From: Ni, Ruiyu > > Sent: Wednesday, March 07, 2018 11:17 AM > > To: Wu, Hao A ; edk2-devel@lists.01.org > > Cc: Wang, Jian J ; Zeng, Star ; > > Dong, Eric ; Yao, Jiewen > > Subject: Re: [PATCH v2 0/2] Resolve feature conflict between NX and Stack > > guard > > > > On 3/6/2018 9:33 PM, Hao Wu wrote: > > > V2 changes: > > > > > > A. Use Hoblib APIs to get the base of stack from Hob. > > > B. Remove unnecessary local variable used in function > > > InitializeDxeNxMemoryProtectionPolicy(). > > > > > > V1 history: > > > > > > If enabled, NX memory protection feature will mark some types of active > > > memory as NX (non-executable), which includes the first page of the stack. > > > This will overwrite the attributes of the first page of the stack if the > > > stack guard feature is also enabled. > > > > > > The series will override the attributes setting to the first page of the > > > stack by adding back the 'EFI_MEMORY_RP' attribute when the stack guard > > > feature is enabled. > > > > > > Cc: Jian J Wang > > > Cc: Star Zeng > > > Cc: Eric Dong > > > Cc: Jiewen Yao > > > Cc: Ruiyu Ni > > > > > > Hao Wu (2): > > >MdeModulePkg/Core: Refine handling NULL detection in NX setting > > >MdeModulePkg/Core: Fix feature conflict between NX and Stack guard > > > > > > MdeModulePkg/Core/Dxe/DxeMain.inf | 4 +- > > > MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 74 > > +++ > > > 2 files changed, 67 insertions(+), 11 deletions(-) > > > > > > >if (MemoryMapEntry->PhysicalStart == 0 && > >PcdGet8 (PcdNullPointerDetectionPropertyMask) != 0) { > > > > ASSERT (MemoryMapEntry->NumberOfPages > 0); > > // > > // Add EFI_MEMORY_RP attribute for page 0 if NULL pointer > > detection is > > // enabled. > > // > > [Ray] 1. I prefer to move the above comments before the "if (...)". > > > > SetUefiImageMemoryAttributes ( > >0, > >EFI_PAGES_TO_SIZE (1), > >EFI_MEMORY_RP | Attributes); > >} > > > >if (StackBase != 0 && > >(StackBase >= MemoryMapEntry->PhysicalStart && > > StackBase < MemoryMapEntry->PhysicalStart + > > LShiftU64 > (MemoryMapEntry->NumberOfPages, > > EFI_PAGE_SHIFT)) && > >PcdGetBool (PcdCpuStackGuard)) { > > > > // > > // Add EFI_MEMORY_RP attribute for the first page of the stack > > if stack > > // guard is enabled. > > // > > SetUefiImageMemoryAttributes ( > >StackBase, > >EFI_PAGES_TO_SIZE (1), > >EFI_MEMORY_RP | Attributes); > > [Ray] 2. The StackBase is directly used here. So do we need to check > > whether it's page aligned? Do we need to check whether the range > > [StackBase, StackBase + 4KB) is inside the MemoryMapEntry? > >} > > If PcdCpuStackGuard is TRUE, I think the owner who allocates memory for > StackBase > should make sure all the conditions you mentioned, but not here. > > > > > -- > > Thanks, > > Ray ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 0/2] Resolve feature conflict between NX and Stack guard
Regards, Jian > -Original Message- > From: Ni, Ruiyu > Sent: Wednesday, March 07, 2018 11:17 AM > To: Wu, Hao A ; edk2-devel@lists.01.org > Cc: Wang, Jian J ; Zeng, Star ; > Dong, Eric ; Yao, Jiewen > Subject: Re: [PATCH v2 0/2] Resolve feature conflict between NX and Stack > guard > > On 3/6/2018 9:33 PM, Hao Wu wrote: > > V2 changes: > > > > A. Use Hoblib APIs to get the base of stack from Hob. > > B. Remove unnecessary local variable used in function > > InitializeDxeNxMemoryProtectionPolicy(). > > > > V1 history: > > > > If enabled, NX memory protection feature will mark some types of active > > memory as NX (non-executable), which includes the first page of the stack. > > This will overwrite the attributes of the first page of the stack if the > > stack guard feature is also enabled. > > > > The series will override the attributes setting to the first page of the > > stack by adding back the 'EFI_MEMORY_RP' attribute when the stack guard > > feature is enabled. > > > > Cc: Jian J Wang > > Cc: Star Zeng > > Cc: Eric Dong > > Cc: Jiewen Yao > > Cc: Ruiyu Ni > > > > Hao Wu (2): > >MdeModulePkg/Core: Refine handling NULL detection in NX setting > >MdeModulePkg/Core: Fix feature conflict between NX and Stack guard > > > > MdeModulePkg/Core/Dxe/DxeMain.inf | 4 +- > > MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 74 > +++ > > 2 files changed, 67 insertions(+), 11 deletions(-) > > > >if (MemoryMapEntry->PhysicalStart == 0 && >PcdGet8 (PcdNullPointerDetectionPropertyMask) != 0) { > > ASSERT (MemoryMapEntry->NumberOfPages > 0); > // > // Add EFI_MEMORY_RP attribute for page 0 if NULL pointer > detection is > // enabled. > // > [Ray] 1. I prefer to move the above comments before the "if (...)". > > SetUefiImageMemoryAttributes ( >0, >EFI_PAGES_TO_SIZE (1), >EFI_MEMORY_RP | Attributes); >} > >if (StackBase != 0 && >(StackBase >= MemoryMapEntry->PhysicalStart && > StackBase < MemoryMapEntry->PhysicalStart + > LShiftU64 (MemoryMapEntry->NumberOfPages, > EFI_PAGE_SHIFT)) && >PcdGetBool (PcdCpuStackGuard)) { > > // > // Add EFI_MEMORY_RP attribute for the first page of the stack > if stack > // guard is enabled. > // > SetUefiImageMemoryAttributes ( >StackBase, >EFI_PAGES_TO_SIZE (1), >EFI_MEMORY_RP | Attributes); > [Ray] 2. The StackBase is directly used here. So do we need to check > whether it's page aligned? Do we need to check whether the range > [StackBase, StackBase + 4KB) is inside the MemoryMapEntry? >} If PcdCpuStackGuard is TRUE, I think the owner who allocates memory for StackBase should make sure all the conditions you mentioned, but not here. > > -- > Thanks, > Ray ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 0/2] Resolve feature conflict between NX and Stack guard
On 3/6/2018 9:33 PM, Hao Wu wrote: V2 changes: A. Use Hoblib APIs to get the base of stack from Hob. B. Remove unnecessary local variable used in function InitializeDxeNxMemoryProtectionPolicy(). V1 history: If enabled, NX memory protection feature will mark some types of active memory as NX (non-executable), which includes the first page of the stack. This will overwrite the attributes of the first page of the stack if the stack guard feature is also enabled. The series will override the attributes setting to the first page of the stack by adding back the 'EFI_MEMORY_RP' attribute when the stack guard feature is enabled. Cc: Jian J Wang Cc: Star Zeng Cc: Eric Dong Cc: Jiewen Yao Cc: Ruiyu Ni Hao Wu (2): MdeModulePkg/Core: Refine handling NULL detection in NX setting MdeModulePkg/Core: Fix feature conflict between NX and Stack guard MdeModulePkg/Core/Dxe/DxeMain.inf | 4 +- MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 74 +++ 2 files changed, 67 insertions(+), 11 deletions(-) if (MemoryMapEntry->PhysicalStart == 0 && PcdGet8 (PcdNullPointerDetectionPropertyMask) != 0) { ASSERT (MemoryMapEntry->NumberOfPages > 0); // // Add EFI_MEMORY_RP attribute for page 0 if NULL pointer detection is // enabled. // [Ray] 1. I prefer to move the above comments before the "if (...)". SetUefiImageMemoryAttributes ( 0, EFI_PAGES_TO_SIZE (1), EFI_MEMORY_RP | Attributes); } if (StackBase != 0 && (StackBase >= MemoryMapEntry->PhysicalStart && StackBase < MemoryMapEntry->PhysicalStart + LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT)) && PcdGetBool (PcdCpuStackGuard)) { // // Add EFI_MEMORY_RP attribute for the first page of the stack if stack // guard is enabled. // SetUefiImageMemoryAttributes ( StackBase, EFI_PAGES_TO_SIZE (1), EFI_MEMORY_RP | Attributes); [Ray] 2. The StackBase is directly used here. So do we need to check whether it's page aligned? Do we need to check whether the range [StackBase, StackBase + 4KB) is inside the MemoryMapEntry? } -- Thanks, Ray ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 0/2] Resolve feature conflict between NX and Stack guard
Thanks for fixing the issue. For this patch series: Reviewed-by: Jian J Wang > -Original Message- > From: Wu, Hao A > Sent: Tuesday, March 06, 2018 9:33 PM > To: edk2-devel@lists.01.org > Cc: Wu, Hao A ; Wang, Jian J ; > Zeng, Star ; Dong, Eric ; Yao, > Jiewen ; Ni, Ruiyu > Subject: [PATCH v2 0/2] Resolve feature conflict between NX and Stack guard > > V2 changes: > > A. Use Hoblib APIs to get the base of stack from Hob. > B. Remove unnecessary local variable used in function >InitializeDxeNxMemoryProtectionPolicy(). > > V1 history: > > If enabled, NX memory protection feature will mark some types of active > memory as NX (non-executable), which includes the first page of the stack. > This will overwrite the attributes of the first page of the stack if the > stack guard feature is also enabled. > > The series will override the attributes setting to the first page of the > stack by adding back the 'EFI_MEMORY_RP' attribute when the stack guard > feature is enabled. > > Cc: Jian J Wang > Cc: Star Zeng > Cc: Eric Dong > Cc: Jiewen Yao > Cc: Ruiyu Ni > > Hao Wu (2): > MdeModulePkg/Core: Refine handling NULL detection in NX setting > MdeModulePkg/Core: Fix feature conflict between NX and Stack guard > > MdeModulePkg/Core/Dxe/DxeMain.inf | 4 +- > MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 74 > +++ > 2 files changed, 67 insertions(+), 11 deletions(-) > > -- > 2.12.0.windows.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH v2 0/2] Resolve feature conflict between NX and Stack guard
V2 changes: A. Use Hoblib APIs to get the base of stack from Hob. B. Remove unnecessary local variable used in function InitializeDxeNxMemoryProtectionPolicy(). V1 history: If enabled, NX memory protection feature will mark some types of active memory as NX (non-executable), which includes the first page of the stack. This will overwrite the attributes of the first page of the stack if the stack guard feature is also enabled. The series will override the attributes setting to the first page of the stack by adding back the 'EFI_MEMORY_RP' attribute when the stack guard feature is enabled. Cc: Jian J Wang Cc: Star Zeng Cc: Eric Dong Cc: Jiewen Yao Cc: Ruiyu Ni Hao Wu (2): MdeModulePkg/Core: Refine handling NULL detection in NX setting MdeModulePkg/Core: Fix feature conflict between NX and Stack guard MdeModulePkg/Core/Dxe/DxeMain.inf | 4 +- MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 74 +++ 2 files changed, 67 insertions(+), 11 deletions(-) -- 2.12.0.windows.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel