Clarify my word below:
Can you just predict and pre-allocate additional pages for future split in
worst case?
If new one need split, you can just use the additional pages.
If not, you can free them later.
For example, you can allocate 2M aligned memory for new pages, for make sure
they are in same page directory.
I think it can resolve the problem.
The key is to pre-calculate how many pages are involved in the worst case.
Thank you
Yao Jiewen
> -----Original Message-----
> From: Wang, Jian J
> Sent: Thursday, November 30, 2017 9:47 AM
> To: Yao, Jiewen <jiewen....@intel.com>
> Cc: edk2-devel@lists.01.org
> Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
>
> Good idea. I think it will highly reduce the possibility but not solve it
> still.
> The dilemma here is: do we need to take care of it? This issue exists in
> theory
> but has not yet been encountered in practice.
>
> > -----Original Message-----
> > From: Yao, Jiewen
> > Sent: Thursday, November 30, 2017 9:36 AM
> > To: Wang, Jian J <jian.j.w...@intel.com>
> > Cc: edk2-devel@lists.01.org
> > Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> >
> > Can you just allocate 1 more page for split?
> > If new one need split, you can just use the additional page.
> > If not, you can free it later.
> >
> > Thank you
> > Yao Jiewen
> >
> >
> > > -----Original Message-----
> > > From: Wang, Jian J
> > > Sent: Thursday, November 30, 2017 9:17 AM
> > > To: Yao, Jiewen <jiewen....@intel.com>
> > > Cc: edk2-devel@lists.01.org
> > > Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> > >
> > > When you split page tables, you need to allocate new pages for new page
> > table.
> > > Since you have new page tables added, you need to mark them to be
> read-only
> > > as well, right? When you do this, the page table for the memory newly
> > allocated
> > > might still needs to be split. This is the worst case but there's still
> > > chance of it,
> > > right? We cannot guarantee, theoretically, the page table for the newly
> > allocated
> > > page tables is in the same as just split ones. So, in worst case, once we
> > > want
> to
> > > mark new page table to be read-only, we need to split the page table
> > managing
> > > those memory, and if we need to do split, we need to allocate more new
> > pages.
> > > This might fall into a recursive situation until all the smallest page
> > > table are
> > > created.
> > > In practice it's almost impossible to let it happen but the chances are
> > > we will
> > fall
> > > into
> > > such recursive situation more than one level.
> > >
> > > > -----Original Message-----
> > > > From: Yao, Jiewen
> > > > Sent: Thursday, November 30, 2017 8:52 AM
> > > > To: Wang, Jian J <jian.j.w...@intel.com>
> > > > Cc: edk2-devel@lists.01.org
> > > > Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> > > >
> > > > -- whenever you're trying to mark one page used as page table to be
> > > > read-
> > only,
> > > > you need to split its page table in advance
> > > > [Jiewen] Sorry, I do not quite understand the problem statement.
> > > > To set a page to be ReadOnly, you just flip the R/W bit in the page
> > > > entry.
> > > > The step I expect is:
> > > > 1) You split page table.
> > > > 2) Fill the data structure in the new entry.
> > > > 3) Flip R/W bit (<== This is the new step).
> > > > 4) FlushPageTable.
> > > >
> > > > Thank you
> > > > Yao Jiewen
> > > >
> > > > > -----Original Message-----
> > > > > From: Wang, Jian J
> > > > > Sent: Thursday, November 30, 2017 8:44 AM
> > > > > To: Yao, Jiewen <jiewen....@intel.com>
> > > > > Cc: edk2-devel@lists.01.org
> > > > > Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> > > > >
> > > > > I did think of protecting new page table in cpu driver. But I think
> > > > > there's
> > risks
> > > > > to do it, which is that there might be a chance that, whenever you're
> trying
> > to
> > > > > mark one page used as page table to be read-only, you need to split
> > > > > its
> > page
> > > > > table in advance, and again and again, until all page tables are for
> > > > > 4KB
> > pages.
> > > > >
> > > > > Although this is a rare case, or the page table "split" will just
> > > > > happen for a
> > few
> > > > > times, it will slow down the boot process a little bit anyway. So I
> > > > > though
> > it's
> > > > > safer not to protect new page tables created after DxeIpl.
> > > > >
> > > > > Maybe it's just over worrying about it. Maybe we just need to add a
> > > > > PCD
> to
> > > > > turn on/off it just in case. Do you have any ideas in mind?
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Yao, Jiewen
> > > > > > Sent: Wednesday, November 29, 2017 9:35 PM
> > > > > > To: Wang, Jian J <jian.j.w...@intel.com>
> > > > > > Cc: edk2-devel@lists.01.org
> > > > > > Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
> > > > > >
> > > > > > I do think we need update CPU driver to protect new allocated split
> page.
> > > > > >
> > > > > > If you verified in shell env, I think it will exposed, please add a
> > > > > > test to
> > trigger
> > > > > > page split in CPU driver.
> > > > > >
> > > > > > I recommend to write some unit test to parse page table in shell.
> > > > > >
> > > > > > thank you!
> > > > > > Yao, Jiewen
> > > > > >
> > > > > >
> > > > > > > 在 2017年11月29日，下午8:15，Wang, Jian J
> > <jian.j.w...@intel.com>
> > > > > 写
> > > > > > 道：
> > > > > > >
> > > > > > > It's in the DxeIplPeim.
> > > > > > >
> > > > > > > By the way, there's an issue in this patch. I forgot to protect
> > > > > > > page table
> > for
> > > > > 32-
> > > > > > bit mode.
> > > > > > > So this patch works only for 64-bit mode. I'll add it in v2 patch.
> > > > > > >
> > > > > > >> -----Original Message-----
> > > > > > >> From: Yao, Jiewen
> > > > > > >> Sent: Wednesday, November 29, 2017 6:56 PM
> > > > > > >> To: Wang, Jian J <jian.j.w...@intel.com>
> > > > > > >> Cc: edk2-devel@lists.01.org
> > > > > > >> Subject: Re: [edk2] [PATCH 0/2] Enable page table write
> > > > > > >> protection
> > > > > > >>
> > > > > > >> Is this code in CPU driver?
> > > > > > >>
> > > > > > >> thank you!
> > > > > > >> Yao, Jiewen
> > > > > > >>
> > > > > > >>
> > > > > > >>> 在 2017年11月29日，下午6:24，Wang, Jian J
> > > > <jian.j.w...@intel.com>
> > > > > > 写
> > > > > > >> 道：
> > > > > > >>>
> > > > > > >>> Yes, I validated them manually with JTAG debug tool.
> > > > > > >>>
> > > > > > >>> if ((L3PageTable[Index3] & IA32_PG_PS) != 0) {
> > > > > > >>> // 1G page. Split to 2M.
> > > > > > >>> L2PageTable = AllocatePages (1);
> > > > > > >>> ASSERT (L2PageTable != NULL);
> > > > > > >>> PhysicalAddress = L3PageTable[Index3] &
> > > > > PAGING_1G_ADDRESS_MASK_64;
> > > > > > >>> for (Index = 0; Index < EFI_PAGE_SIZE/sizeof (UINT64);
> > > > > > >>> ++Index)
> {
> > > > > > >>> L2PageTable[Index] = PhysicalAddress | AddressEncMask |
> > > > > > >>> IA32_PG_PS | IA32_PG_P |
> > > IA32_PG_RW;
> > > > > > >>> PhysicalAddress += SIZE_2MB;
> > > > > > >>> }
> > > > > > >>> L3PageTable[Index3] = (UINT64) (UINTN) L2PageTable |
> > > > > AddressEncMask |
> > > > > > >>> IA32_PG_P |
> > > > > IA32_PG_RW;
> > > > > > >>> SetPageReadOnly (PageTableBase,
> > > > > > >> (EFI_PHYSICAL_ADDRESS)(UINTN)L2PageTable);
> > > > > > >>> }
> > > > > > >>>
> > > > > > >>> The newly allocated page table is set in the SetPageReadOnly()
> > > > > > >>> itself
> > > > > > >> recursively, like
> > > > > > >>> above code in which L2PageTable is allocated and then set it to
> > > > > > >>> be
> > > > > read-only
> > > > > > >> after
> > > > > > >>> initializing the table content.
> > > > > > >>>
> > > > > > >>>> -----Original Message-----
> > > > > > >>>> From: Yao, Jiewen
> > > > > > >>>> Sent: Wednesday, November 29, 2017 5:16 PM
> > > > > > >>>> To: Wang, Jian J <jian.j.w...@intel.com>
> > > > > > >>>> Cc: edk2-devel@lists.01.org
> > > > > > >>>> Subject: Re: [edk2] [PATCH 0/2] Enable page table write
> protection
> > > > > > >>>>
> > > > > > >>>> Thanks.
> > > > > > >>>>
> > > > > > >>>> May I know if this is validated in uefi shell, that all page
> > > > > > >>>> table is
> > > > readonly?
> > > > > > >>>>
> > > > > > >>>> I did not find the code to set new allocated split page to be
> > readonly.
> > > > Can
> > > > > > you
> > > > > > >>>> give me a hand on that?
> > > > > > >>>>
> > > > > > >>>> thank you!
> > > > > > >>>> Yao, Jiewen
> > > > > > >>>>
> > > > > > >>>>
> > > > > > >>>>> 在 2017年11月29日，下午4:47，Jian J Wang
> > > > > <jian.j.w...@intel.com>
> > > > > > >> 写
> > > > > > >>>> 道：
> > > > > > >>>>>
> > > > > > >>>>> Write Protect feature (CR0.WP) is always enabled in driver
> > > > > > >>>> UefiCpuPkg/CpuDxe.
> > > > > > >>>>> But the memory pages used for page table are not set as
> read-only
> > > in
> > > > > the
> > > > > > >>>> driver
> > > > > > >>>>> DxeIplPeim, after the paging is setup. This might jeopardize
> > > > > > >>>>> the
> > page
> > > > > > table
> > > > > > >>>>> integrity if there's buffer overflow occured in other part of
> system.
> > > > > > >>>>>
> > > > > > >>>>> This patch series will change this situation by clearing R/W
> > > > > > >>>>> bit in
> > > page
> > > > > > >> attribute
> > > > > > >>>>> of the pages used as page table.
> > > > > > >>>>>
> > > > > > >>>>> Validation works include booting Windows (10/server 2016) and
> > > Linux
> > > > > > >>>> (Fedora/Ubuntu)
> > > > > > >>>>> on OVMF and Intel real platform.
> > > > > > >>>>>
> > > > > > >>>>> Jian J Wang (2):
> > > > > > >>>>> UefiCpuPkg/CpuDxe: Check CR0.WP before changing page table
> > > > > > >>>>> MdeModulePkg/DxeIpl: Mark page table as read-only
> > > > > > >>>>>
> > > > > > >>>>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 166
> > > > > > >>>> +++++++++++++++++++++++
> > > > > > >>>>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h | 14
> ++
> > > > > > >>>>> UefiCpuPkg/CpuDxe/CpuPageTable.c | 65
> > > > > ++++++++-
> > > > > > >>>>> 3 files changed, 241 insertions(+), 4 deletions(-)
> > > > > > >>>>>
> > > > > > >>>>> --
> > > > > > >>>>> 2.14.1.windows.1
> > > > > > >>>>>
> > > > > > >>>>> _______________________________________________
> > > > > > >>>>> edk2-devel mailing list
> > > > > > >>>>> edk2-devel@lists.01.org
> > > > > > >>>>> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
