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 <[email protected]> > Cc: [email protected] > 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 <[email protected]> 写 > 道： > > > > 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 <[email protected]> > >> Cc: [email protected] > >> 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 <[email protected]> > 写 > >> 道： > >>> > >>> 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 <[email protected]> > >>>> Cc: [email protected] > >>>> 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 <[email protected]> > >> 写 > >>>> 道： > >>>>> > >>>>> 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 > >>>>> [email protected] > >>>>> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
