Re: [edk2-devel] [PATCH 1/1] MdeModulePkg/DxeIplPeim: Relocate the operation of PageMapLevel5Entry++
> -Original Message-
> From: Zhang, Shenglei
> Sent: Monday, August 12, 2019 2:25 PM
> To: devel@edk2.groups.io
> Cc: Bi, Dandan; Gao, Liming; Wu, Hao A; Laszlo Ersek; Dong, Eric
> Subject: [PATCH 1/1] MdeModulePkg/DxeIplPeim: Relocate the operation of
> PageMapLevel5Entry++
>
> PageMapLevel5Entry may be uninitialized in original code, which means
> uninitialized pointer will be modified at some circumstance.
> So relocate the operation of PageMapLevel5Entry++ in order to make sure
> the pointer could be modified only when it is uninitialized.
With the commit message updated, pushed via commit 46f8a68916.
Best Regards,
Hao Wu
>
> Cc: Dandan Bi
> Cc: Liming Gao
> Cc: Hao A Wu
> Cc: Laszlo Ersek
> Cc: Eric Dong
> Signed-off-by: Shenglei Zhang
> ---
> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> index b40b7e0c9813..2389f3eb485b 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> @@ -764,7 +764,7 @@ CreateIdentityMappingPageTables (
>
>for ( IndexOfPml5Entries = 0
>; IndexOfPml5Entries < NumberOfPml5EntriesNeeded
> - ; IndexOfPml5Entries++, PageMapLevel5Entry++) {
> + ; IndexOfPml5Entries++) {
> //
> // Each PML5 entry points to a page of PML4 entires.
> // So lets allocate space for them and fill them in in the
> IndexOfPml4Entries loop.
> @@ -780,6 +780,7 @@ CreateIdentityMappingPageTables (
>PageMapLevel5Entry->Uint64 = (UINT64) (UINTN) PageMapLevel4Entry |
> AddressEncMask;
>PageMapLevel5Entry->Bits.ReadWrite = 1;
>PageMapLevel5Entry->Bits.Present = 1;
> + PageMapLevel5Entry++;
> }
>
> for ( IndexOfPml4Entries = 0
> --
> 2.18.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#45577): https://edk2.groups.io/g/devel/message/45577
Mute This Topic: https://groups.io/mt/32838114/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/1] MdeModulePkg/DxeIplPeim: Relocate the operation of PageMapLevel5Entry++
On 08/12/19 08:24, Zhang, Shenglei wrote:
> PageMapLevel5Entry may be uninitialized in original code, which means
> uninitialized pointer will be modified at some circumstance.
> So relocate the operation of PageMapLevel5Entry++ in order to make sure
> the pointer could be modified only when it is uninitialized.
>
> Cc: Dandan Bi
> Cc: Liming Gao
> Cc: Hao A Wu
> Cc: Laszlo Ersek
> Cc: Eric Dong
> Signed-off-by: Shenglei Zhang
> ---
> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> index b40b7e0c9813..2389f3eb485b 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> @@ -764,7 +764,7 @@ CreateIdentityMappingPageTables (
>
>for ( IndexOfPml5Entries = 0
>; IndexOfPml5Entries < NumberOfPml5EntriesNeeded
> - ; IndexOfPml5Entries++, PageMapLevel5Entry++) {
> + ; IndexOfPml5Entries++) {
> //
> // Each PML5 entry points to a page of PML4 entires.
> // So lets allocate space for them and fill them in in the
> IndexOfPml4Entries loop.
> @@ -780,6 +780,7 @@ CreateIdentityMappingPageTables (
>PageMapLevel5Entry->Uint64 = (UINT64) (UINTN) PageMapLevel4Entry |
> AddressEncMask;
>PageMapLevel5Entry->Bits.ReadWrite = 1;
>PageMapLevel5Entry->Bits.Present = 1;
> + PageMapLevel5Entry++;
> }
>
> for ( IndexOfPml4Entries = 0
>
I'm commenting for a second time here because the underlying bug seems
to break the building of OVMF:
[edk2-devel] ovmf build fail with gcc 4.8.5
When you commit the patch, please add the following to the commit message:
Fixes: b3527dedc3951f061c5a73cb4fb2b0f95f47e08b
(Because the invalid access was apparently introduced in that commit. I
did regression-test b3527dedc395, but I didn't get the warning.)
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#45563): https://edk2.groups.io/g/devel/message/45563
Mute This Topic: https://groups.io/mt/32838114/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/1] MdeModulePkg/DxeIplPeim: Relocate the operation of PageMapLevel5Entry++
Agree with Dandan's comments. With that update, Reviewed-by: Eric Dong
> -Original Message-
> From: Bi, Dandan
> Sent: Tuesday, August 13, 2019 10:04 AM
> To: Zhang, Shenglei ; devel@edk2.groups.io
> Cc: Gao, Liming ; Wu, Hao A ;
> Laszlo Ersek ; Dong, Eric
> Subject: RE: [PATCH 1/1] MdeModulePkg/DxeIplPeim: Relocate the
> operation of PageMapLevel5Entry++
>
> > -Original Message-
> > From: Zhang, Shenglei
> > Sent: Monday, August 12, 2019 2:25 PM
> > To: devel@edk2.groups.io
> > Cc: Bi, Dandan ; Gao, Liming
> > ; Wu, Hao A ; Laszlo Ersek
> > ; Dong, Eric
> > Subject: [PATCH 1/1] MdeModulePkg/DxeIplPeim: Relocate the operation
> > of
> > PageMapLevel5Entry++
> >
> > PageMapLevel5Entry may be uninitialized in original code, which means
> > uninitialized pointer will be modified at some circumstance.
> > So relocate the operation of PageMapLevel5Entry++ in order to make
> > sure the pointer could be modified only when it is uninitialized.
> I think "uninitialized" here should be a typo, you may mean "initialized".
> Please update it before commit.
> I have no other comments for this patch.
> Reviewed-by: Dandan Bi
>
> >
> > Cc: Dandan Bi
> > Cc: Liming Gao
> > Cc: Hao A Wu
> > Cc: Laszlo Ersek
> > Cc: Eric Dong
> > Signed-off-by: Shenglei Zhang
> > ---
> > MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> > b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> > index b40b7e0c9813..2389f3eb485b 100644
> > --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> > +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> > @@ -764,7 +764,7 @@ CreateIdentityMappingPageTables (
> >
> >for ( IndexOfPml5Entries = 0
> >; IndexOfPml5Entries < NumberOfPml5EntriesNeeded
> > - ; IndexOfPml5Entries++, PageMapLevel5Entry++) {
> > + ; IndexOfPml5Entries++) {
> > //
> > // Each PML5 entry points to a page of PML4 entires.
> > // So lets allocate space for them and fill them in in the
> > IndexOfPml4Entries loop.
> > @@ -780,6 +780,7 @@ CreateIdentityMappingPageTables (
> >PageMapLevel5Entry->Uint64 = (UINT64) (UINTN)
> > PageMapLevel4Entry | AddressEncMask;
> >PageMapLevel5Entry->Bits.ReadWrite = 1;
> >PageMapLevel5Entry->Bits.Present = 1;
> > + PageMapLevel5Entry++;
> > }
> >
> > for ( IndexOfPml4Entries = 0
> > --
> > 2.18.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#45487): https://edk2.groups.io/g/devel/message/45487
Mute This Topic: https://groups.io/mt/32838114/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/1] MdeModulePkg/DxeIplPeim: Relocate the operation of PageMapLevel5Entry++
> -Original Message-
> From: Zhang, Shenglei
> Sent: Monday, August 12, 2019 2:25 PM
> To: devel@edk2.groups.io
> Cc: Bi, Dandan ; Gao, Liming ;
> Wu, Hao A ; Laszlo Ersek ; Dong,
> Eric
> Subject: [PATCH 1/1] MdeModulePkg/DxeIplPeim: Relocate the operation of
> PageMapLevel5Entry++
>
> PageMapLevel5Entry may be uninitialized in original code, which means
> uninitialized pointer will be modified at some circumstance.
> So relocate the operation of PageMapLevel5Entry++ in order to make sure
> the pointer could be modified only when it is uninitialized.
I think "uninitialized" here should be a typo, you may mean "initialized".
Please update it before commit.
I have no other comments for this patch.
Reviewed-by: Dandan Bi
>
> Cc: Dandan Bi
> Cc: Liming Gao
> Cc: Hao A Wu
> Cc: Laszlo Ersek
> Cc: Eric Dong
> Signed-off-by: Shenglei Zhang
> ---
> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> index b40b7e0c9813..2389f3eb485b 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> @@ -764,7 +764,7 @@ CreateIdentityMappingPageTables (
>
>for ( IndexOfPml5Entries = 0
>; IndexOfPml5Entries < NumberOfPml5EntriesNeeded
> - ; IndexOfPml5Entries++, PageMapLevel5Entry++) {
> + ; IndexOfPml5Entries++) {
> //
> // Each PML5 entry points to a page of PML4 entires.
> // So lets allocate space for them and fill them in in the
> IndexOfPml4Entries loop.
> @@ -780,6 +780,7 @@ CreateIdentityMappingPageTables (
>PageMapLevel5Entry->Uint64 = (UINT64) (UINTN) PageMapLevel4Entry |
> AddressEncMask;
>PageMapLevel5Entry->Bits.ReadWrite = 1;
>PageMapLevel5Entry->Bits.Present = 1;
> + PageMapLevel5Entry++;
> }
>
> for ( IndexOfPml4Entries = 0
> --
> 2.18.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#45485): https://edk2.groups.io/g/devel/message/45485
Mute This Topic: https://groups.io/mt/32838114/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/1] MdeModulePkg/DxeIplPeim: Relocate the operation of PageMapLevel5Entry++
Hello Eric,
Could you help to take a look at this one as well?
> -Original Message-
> From: Zhang, Shenglei
> Sent: Monday, August 12, 2019 2:25 PM
> To: devel@edk2.groups.io
> Cc: Bi, Dandan; Gao, Liming; Wu, Hao A; Laszlo Ersek; Dong, Eric
> Subject: [PATCH 1/1] MdeModulePkg/DxeIplPeim: Relocate the operation of
> PageMapLevel5Entry++
>
> PageMapLevel5Entry may be uninitialized in original code, which means
> uninitialized pointer will be modified at some circumstance.
> So relocate the operation of PageMapLevel5Entry++ in order to make sure
> the pointer could be modified only when it is uninitialized.
>
> Cc: Dandan Bi
> Cc: Liming Gao
> Cc: Hao A Wu
> Cc: Laszlo Ersek
> Cc: Eric Dong
> Signed-off-by: Shenglei Zhang
> ---
> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> index b40b7e0c9813..2389f3eb485b 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> @@ -764,7 +764,7 @@ CreateIdentityMappingPageTables (
>
>for ( IndexOfPml5Entries = 0
>; IndexOfPml5Entries < NumberOfPml5EntriesNeeded
> - ; IndexOfPml5Entries++, PageMapLevel5Entry++) {
> + ; IndexOfPml5Entries++) {
> //
> // Each PML5 entry points to a page of PML4 entires.
> // So lets allocate space for them and fill them in in the
> IndexOfPml4Entries loop.
> @@ -780,6 +780,7 @@ CreateIdentityMappingPageTables (
>PageMapLevel5Entry->Uint64 = (UINT64) (UINTN) PageMapLevel4Entry |
> AddressEncMask;
>PageMapLevel5Entry->Bits.ReadWrite = 1;
>PageMapLevel5Entry->Bits.Present = 1;
> + PageMapLevel5Entry++;
> }
I think the change is fine,
Reviewed-by: Hao A Wu
Best Regards,
Hao Wu
>
> for ( IndexOfPml4Entries = 0
> --
> 2.18.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#45480): https://edk2.groups.io/g/devel/message/45480
Mute This Topic: https://groups.io/mt/32838114/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/1] MdeModulePkg/DxeIplPeim: Relocate the operation of PageMapLevel5Entry++
Hello Shenglei,
On 08/12/19 08:24, Shenglei Zhang wrote:
> PageMapLevel5Entry may be uninitialized in original code, which means
> uninitialized pointer will be modified at some circumstance.
> So relocate the operation of PageMapLevel5Entry++ in order to make sure
> the pointer could be modified only when it is uninitialized.
>
> Cc: Dandan Bi
> Cc: Liming Gao
> Cc: Hao A Wu
> Cc: Laszlo Ersek
> Cc: Eric Dong
> Signed-off-by: Shenglei Zhang
> ---
> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> index b40b7e0c9813..2389f3eb485b 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> @@ -764,7 +764,7 @@ CreateIdentityMappingPageTables (
>
>for ( IndexOfPml5Entries = 0
>; IndexOfPml5Entries < NumberOfPml5EntriesNeeded
> - ; IndexOfPml5Entries++, PageMapLevel5Entry++) {
> + ; IndexOfPml5Entries++) {
> //
> // Each PML5 entry points to a page of PML4 entires.
> // So lets allocate space for them and fill them in in the
> IndexOfPml4Entries loop.
> @@ -780,6 +780,7 @@ CreateIdentityMappingPageTables (
>PageMapLevel5Entry->Uint64 = (UINT64) (UINTN) PageMapLevel4Entry |
> AddressEncMask;
>PageMapLevel5Entry->Bits.ReadWrite = 1;
>PageMapLevel5Entry->Bits.Present = 1;
> + PageMapLevel5Entry++;
> }
>
> for ( IndexOfPml4Entries = 0
>
I prefer to leave this to the other people on CC.
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#45433): https://edk2.groups.io/g/devel/message/45433
Mute This Topic: https://groups.io/mt/32838114/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH 1/1] MdeModulePkg/DxeIplPeim: Relocate the operation of PageMapLevel5Entry++
PageMapLevel5Entry may be uninitialized in original code, which means
uninitialized pointer will be modified at some circumstance.
So relocate the operation of PageMapLevel5Entry++ in order to make sure
the pointer could be modified only when it is uninitialized.
Cc: Dandan Bi
Cc: Liming Gao
Cc: Hao A Wu
Cc: Laszlo Ersek
Cc: Eric Dong
Signed-off-by: Shenglei Zhang
---
MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
index b40b7e0c9813..2389f3eb485b 100644
--- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
+++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
@@ -764,7 +764,7 @@ CreateIdentityMappingPageTables (
for ( IndexOfPml5Entries = 0
; IndexOfPml5Entries < NumberOfPml5EntriesNeeded
- ; IndexOfPml5Entries++, PageMapLevel5Entry++) {
+ ; IndexOfPml5Entries++) {
//
// Each PML5 entry points to a page of PML4 entires.
// So lets allocate space for them and fill them in in the
IndexOfPml4Entries loop.
@@ -780,6 +780,7 @@ CreateIdentityMappingPageTables (
PageMapLevel5Entry->Uint64 = (UINT64) (UINTN) PageMapLevel4Entry |
AddressEncMask;
PageMapLevel5Entry->Bits.ReadWrite = 1;
PageMapLevel5Entry->Bits.Present = 1;
+ PageMapLevel5Entry++;
}
for ( IndexOfPml4Entries = 0
--
2.18.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#45401): https://edk2.groups.io/g/devel/message/45401
Mute This Topic: https://groups.io/mt/32838114/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
