Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix issue that IsModified is wrongly set in PageTableMap

["Laszlo Ersek" <lersek@redhat.com>]

On 1/10/24 06:38, Zhiguang Liu wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4614
> 
> Fix issue that IsModified is wrongly set in PageTableMap.
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Crystal Lee <CrystalLee@ami.com.tw>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
>  UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> index 36b2c4e6a3..164187f151 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> @@ -567,7 +567,10 @@ PageTableLibMapInLevel (
>          OriginalCurrentPagingEntry.Uint64 = CurrentPagingEntry->Uint64;
>          PageTableLibSetPle (Level, CurrentPagingEntry, Offset, Attribute, &CurrentMask);
>  
> -        if (OriginalCurrentPagingEntry.Uint64 != CurrentPagingEntry->Uint64) {
> +        if (Modify && (OriginalCurrentPagingEntry.Uint64 != CurrentPagingEntry->Uint64)) {
> +          //
> +          // The page table entry can be changed by this function only when Modify is true.
> +          //
>            *IsModified = TRUE;
>          }
>        }
> @@ -609,7 +612,10 @@ PageTableLibMapInLevel (
>    // Check if ParentPagingEntry entry is modified here is enough. Except the changes happen in leaf PagingEntry during
>    // the while loop, if there is any other change happens in page table, the ParentPagingEntry must has been modified.
>    //
> -  if (OriginalParentPagingEntry.Uint64 != ParentPagingEntry->Uint64) {
> +  if (Modify && (OriginalParentPagingEntry.Uint64 != ParentPagingEntry->Uint64)) {
> +    //
> +    // The page table entry can be changed by this function only when Modify is true.
> +    //
>      *IsModified = TRUE;
>    }
>  

This function is incredibly complicated, so reviewing this patch is
hard, even after reading the bugzilla ticket.

The commit message is useless. It should contain a brief description of
the problem, and how the fix resolves the problem.

The documentation of the PageTableLibMapInLevel() function is wrong,
even before this patch. It documents the "IsModified" output-only
parameter as follows:

"TRUE means page table is modified. FALSE means page table is not modified."

This states that "IsModified" is always set on output, to either FALSE
or TRUE. Which is an incorrect statement; in reality the caller is
expected to pre-set (*IsModified) to FALSE, and PageTableLibMapInLevel()
will (conditionally!) perform a FALSE->TRUE transition only.

Now, this patch may fix a bug, but it makes the above-described
documentation issue worse, by further restricting the condition for said
FALSE->TRUE transition.

The fix per se looks vaguely reasonable to me (really the function is so
complicated that verifying this change from scratch would take me ages),
but minimally, the documentation of "IsModified" should certainly be
updated too. To something like this:

  @param[out] IsModified  If "Modify" is TRUE on input and the function
                          has actually modified the page table, then set
                          to TRUE on output. Not overwritten otherwise.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113528): https://edk2.groups.io/g/devel/message/113528
Mute This Topic: https://groups.io/mt/103636407/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-