From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=17.171.2.33; helo=mail-in23.apple.com; envelope-from=afish@apple.com; receiver=edk2-devel@lists.01.org Received: from mail-in23.apple.com (mail-out23.apple.com [17.171.2.33]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id F06EF21A10962 for ; Wed, 29 Nov 2017 17:33:21 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; d=apple.com; s=mailout2048s; c=relaxed/simple; q=dns/txt; i=@apple.com; t=1512005866; h=From:Sender:Reply-To:Subject:Date:Message-id:To:Cc:MIME-version:Content-type: Content-transfer-encoding:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:In-reply-to:References:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=tIBmtZEwRiGgLC714C19ZzCURz4D+OVv2AhWaB6m3k8=; b=AcgDFGQRjztYmcT87fBufuNA0t7D8LxLgFiop9JjiFAZfWxMs88h+U5QdShP2IKl 43yJZnp8kOCNTmn/ZqKlt4MG1dQZD320KwwEkfxcKnCD6w2qSXiG1UjijrAdgx2N ND8wPemHcXM79ey19fxBhQb9ZZOv+HI4P8wd46Kn0X0oEyhNXTQTY1WLu4PQ5Ei9 fpdkOtlHVjhhzMq9ZzA+u4eWMS8MKqCZP6ehayaF6Xcgus0goNErpG6Xq4QzqgZF vr40SRywQkkAcmvasiK/lxTupOKqphu4kD5m0wtyAysyaPwuuJh8imG9KiG6WVFi fRD2W6sD9dhkIEYh6IGV7w==; Received: from relay2.apple.com (relay2.apple.com [17.128.113.67]) (using TLS with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mail-in23.apple.com (Apple Secure Mail Relay) with SMTP id 43.2D.29340.9E06F1A5; Wed, 29 Nov 2017 17:37:46 -0800 (PST) X-AuditID: 11ab0217-df6059c00000729c-58-5a1f60e91a79 Received: from nwk-mmpp-sz13.apple.com (nwk-mmpp-sz13.apple.com [17.128.115.216]) by relay2.apple.com (Apple SCV relay) with SMTP id 64.8D.07440.9E06F1A5; Wed, 29 Nov 2017 17:37:45 -0800 (PST) MIME-version: 1.0 Received: from da0601a-dhcp180.apple.com ([17.226.15.180]) by nwk-mmpp-sz13.apple.com (Oracle Communications Messaging Server 8.0.2.1.20171102 64bit (built Nov 2 2017)) with ESMTPSA id <0P0700DK0J6XXG50@nwk-mmpp-sz13.apple.com>; Wed, 29 Nov 2017 17:37:45 -0800 (PST) Sender: afish@apple.com From: Andrew Fish In-reply-to: Date: Wed, 29 Nov 2017 17:37:44 -0800 Cc: "Yao, Jiewen" , "edk2-devel@lists.01.org" Message-id: References: <20171129084640.20076-1-jian.j.wang@intel.com> <19004429-87DC-4620-8A78-4EF085D5AD9D@intel.com> <47960CCE-CA63-49FE-99AF-48E61050EF05@intel.com> <75B70EA2-DA2E-41E5-9580-FA23AB545AC2@intel.com> <74D8A39837DF1E4DA445A8C0B3885C503AA326DF@shsmsx102.ccr.corp.intel.com> To: "Wang, Jian J" X-Mailer: Apple Mail (2.3445.5.20) X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrALMWRmVeSWpSXmKPExsUi2FDorPsqQT7KoMfDYs+ho8wW877NYLVY 99HDgdlj8Z6XTB7ds/+xBDBFcdmkpOZklqUW6dslcGUcuHWHveCRQ8XTLRtYGhivmXQxcnJI CJhIdB2/x97FyMUhJLCGSeLnhkcsMImtX7cyQyQOMUrsmg+R4BUQlPgx+R6QzcHBLKAuMWVK LkTNZCaJd5/2M4LUCAuIS7w7s4kZwnaSmLNgIhOIzSagLLFi/gd2EJtTIEzi++e5YHEWAVWJ dSuus4HYzALxEo03FrNC2NoST95dYIXYayNx9tliRohlR1kknhxcDLZAREBL4syqFqirlSSm f7/NBlIkIdDBJtH+cQ37BEbhWUgOn4Vw+CwkOxYwMq9iFM5NzMzRzcwzMtZLLCjISdVLzs/d xAgK8tVM4jsYP782PMQowMGoxMN7Q00+Sog1say4MvcQozQHi5I4r7YfUEggPbEkNTs1tSC1 KL6oNCe1+BAjEwenVAMjH896cS9G3YWvVlnOvRn0RXh1b8UuoyVqKef3XzrPMFF9UtOmdVFu E3r22NjcePkgbvLS3DVBdUe6PaLjTs5hWL+XdXqPAKvBppefGdaKbFjo7eDVEVeTbiV/bxd7 04bYNW5xPMcnlLIk/p3Kc+bOyvSVQSyb527duVXcOis5+m992Nw/6xorlViKMxINtZiLihMB pGsk6FMCAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrMLMWRmVeSWpSXmKPExsUi2FB8Q/dlgnyUwZw2BYs9h44yW8z7NoPV Yt1HDwdmj8V7XjJ5dM/+xxLAFMVlk5Kak1mWWqRvl8CVceDWHfaCRw4VT7dsYGlgvGbSxcjJ ISFgIrH161bmLkYuDiGBQ4wSu+Y/YgFJ8AoISvyYfA/I5uBgFlCXmDIlF6JmMpPEu0/7GUFq hAXEJd6d2cQMYTtJzFkwkQnEZhNQllgx/wM7iM0pECbx/fNcsDiLgKrEuhXX2UBsZoF4icYb i1khbG2JJ+8usELstZE4+2wxI8SyoywSTw4uBlsgIqAlcWZVCwvE1UoS07/fZpvAKDALya2z EG6dhWTsAkbmVYwCRak5iZVGeokFBTmpesn5uZsYwUFZ6LyD8dgyq0OMAhyMSjy8N9Tko4RY E8uKK3OBgcHBrCTCa2MGFOJNSaysSi3Kjy8qzUktPsQozcGiJM77aaV4lJBAemJJanZqakFq EUyWiYNTqoGxktdMNeW04qWbJ37siz2aJM3rmrAk7heLahHbL/5vkyR/ib5sCn4080iQsOXO v7znn6/64Jp6XO7Fq8OmLVO/FvG/OjHtWc9X11794knbJFk3XfpyzpHrZPtjHRuB/E7NhT3n nbcaOU247mjdNqO5b+qM4I9KHb/+HLpxqbZIuPxTZ90G/TnuSizFGYmGWsxFxYkA5ilhTkYC AAA= Subject: Re: [PATCH 0/2] Enable page table write protection X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 30 Nov 2017 01:33:22 -0000 Content-type: text/plain; charset=utf-8 Content-transfer-encoding: quoted-printable > On Nov 29, 2017, at 5:16 PM, Wang, Jian J = wrote: >=20 > 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=20 > such recursive situation more than one level. >=20 Why not just have the page table update code turn off the write protect = when it is modifying tables, and then turn it back on? Or am I missing = something? Thanks, Andrew Fish >> -----Original Message----- >> From: Yao, Jiewen >> Sent: Thursday, November 30, 2017 8:52 AM >> To: Wang, Jian J >> Cc: edk2-devel@lists.01.org >> Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection >>=20 >> -- 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 (<=3D=3D This is the new step). >> 4) FlushPageTable. >>=20 >> Thank you >> Yao Jiewen >>=20 >>> -----Original Message----- >>> From: Wang, Jian J >>> Sent: Thursday, November 30, 2017 8:44 AM >>> To: Yao, Jiewen >>> Cc: edk2-devel@lists.01.org >>> Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection >>>=20 >>> 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. >>>=20 >>> 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. >>>=20 >>> 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? >>>=20 >>>> -----Original Message----- >>>> From: Yao, Jiewen >>>> Sent: Wednesday, November 29, 2017 9:35 PM >>>> To: Wang, Jian J >>>> Cc: edk2-devel@lists.01.org >>>> Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection >>>>=20 >>>> I do think we need update CPU driver to protect new allocated split = page. >>>>=20 >>>> If you verified in shell env, I think it will exposed, please add a = test to trigger >>>> page split in CPU driver. >>>>=20 >>>> I recommend to write some unit test to parse page table in shell. >>>>=20 >>>> thank you! >>>> Yao, Jiewen >>>>=20 >>>>=20 >>>>> =E5=9C=A8 2017=E5=B9=B411=E6=9C=8829=E6=97=A5=EF=BC=8C=E4=B8=8B=E5=8D= =888:15=EF=BC=8CWang, Jian J >>> =E5=86=99 >>>> =E9=81=93=EF=BC=9A >>>>>=20 >>>>> It's in the DxeIplPeim. >>>>>=20 >>>>> 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. >>>>>=20 >>>>>> -----Original Message----- >>>>>> From: Yao, Jiewen >>>>>> Sent: Wednesday, November 29, 2017 6:56 PM >>>>>> To: Wang, Jian J >>>>>> Cc: edk2-devel@lists.01.org >>>>>> Subject: Re: [edk2] [PATCH 0/2] Enable page table write = protection >>>>>>=20 >>>>>> Is this code in CPU driver? >>>>>>=20 >>>>>> thank you! >>>>>> Yao, Jiewen >>>>>>=20 >>>>>>=20 >>>>>>> =E5=9C=A8 2017=E5=B9=B411=E6=9C=8829=E6=97=A5=EF=BC=8C=E4=B8=8B=E5= =8D=886:24=EF=BC=8CWang, Jian J >> >>>> =E5=86=99 >>>>>> =E9=81=93=EF=BC=9A >>>>>>>=20 >>>>>>> Yes, I validated them manually with JTAG debug tool. >>>>>>>=20 >>>>>>> if ((L3PageTable[Index3] & IA32_PG_PS) !=3D 0) { >>>>>>> // 1G page. Split to 2M. >>>>>>> L2PageTable =3D AllocatePages (1); >>>>>>> ASSERT (L2PageTable !=3D NULL); >>>>>>> PhysicalAddress =3D L3PageTable[Index3] & >>> PAGING_1G_ADDRESS_MASK_64; >>>>>>> for (Index =3D 0; Index < EFI_PAGE_SIZE/sizeof (UINT64); = ++Index) { >>>>>>> L2PageTable[Index] =3D PhysicalAddress | AddressEncMask | >>>>>>> IA32_PG_PS | IA32_PG_P | IA32_PG_RW; >>>>>>> PhysicalAddress +=3D SIZE_2MB; >>>>>>> } >>>>>>> L3PageTable[Index3] =3D (UINT64) (UINTN) L2PageTable | >>> AddressEncMask | >>>>>>> IA32_PG_P | >>> IA32_PG_RW; >>>>>>> SetPageReadOnly (PageTableBase, >>>>>> (EFI_PHYSICAL_ADDRESS)(UINTN)L2PageTable); >>>>>>> } >>>>>>>=20 >>>>>>> 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. >>>>>>>=20 >>>>>>>> -----Original Message----- >>>>>>>> From: Yao, Jiewen >>>>>>>> Sent: Wednesday, November 29, 2017 5:16 PM >>>>>>>> To: Wang, Jian J >>>>>>>> Cc: edk2-devel@lists.01.org >>>>>>>> Subject: Re: [edk2] [PATCH 0/2] Enable page table write = protection >>>>>>>>=20 >>>>>>>> Thanks. >>>>>>>>=20 >>>>>>>> May I know if this is validated in uefi shell, that all page = table is >> readonly? >>>>>>>>=20 >>>>>>>> I did not find the code to set new allocated split page to be = readonly. >> Can >>>> you >>>>>>>> give me a hand on that? >>>>>>>>=20 >>>>>>>> thank you! >>>>>>>> Yao, Jiewen >>>>>>>>=20 >>>>>>>>=20 >>>>>>>>> =E5=9C=A8 2017=E5=B9=B411=E6=9C=8829=E6=97=A5=EF=BC=8C=E4=B8=8B=E5= =8D=884:47=EF=BC=8CJian J Wang >>> >>>>>> =E5=86=99 >>>>>>>> =E9=81=93=EF=BC=9A >>>>>>>>>=20 >>>>>>>>> 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. >>>>>>>>>=20 >>>>>>>>> This patch series will change this situation by clearing R/W = bit in page >>>>>> attribute >>>>>>>>> of the pages used as page table. >>>>>>>>>=20 >>>>>>>>> Validation works include booting Windows (10/server 2016) and = Linux >>>>>>>> (Fedora/Ubuntu) >>>>>>>>> on OVMF and Intel real platform. >>>>>>>>>=20 >>>>>>>>> Jian J Wang (2): >>>>>>>>> UefiCpuPkg/CpuDxe: Check CR0.WP before changing page table >>>>>>>>> MdeModulePkg/DxeIpl: Mark page table as read-only >>>>>>>>>=20 >>>>>>>>> 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(-) >>>>>>>>>=20 >>>>>>>>> -- >>>>>>>>> 2.14.1.windows.1 >>>>>>>>>=20 >>>>>>>>> _______________________________________________ >>>>>>>>> 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