From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.120; helo=mga04.intel.com; envelope-from=jiewen.yao@intel.com; receiver=edk2-devel@lists.01.org Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) (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 4E89C2034B421 for ; Wed, 3 Jan 2018 17:02:50 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 03 Jan 2018 17:07:15 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,504,1508828400"; d="scan'208";a="7073539" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga007.fm.intel.com with ESMTP; 03 Jan 2018 17:07:15 -0800 Received: from fmsmsx118.amr.corp.intel.com (10.18.116.18) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 3 Jan 2018 17:07:15 -0800 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx118.amr.corp.intel.com (10.18.116.18) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 3 Jan 2018 17:07:14 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.189]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.93]) with mapi id 14.03.0319.002; Thu, 4 Jan 2018 09:07:13 +0800 From: "Yao, Jiewen" To: Paulo Alcantara , "edk2-devel@lists.01.org" CC: Laszlo Ersek , "Dong, Eric" Thread-Topic: [edk2] [RFC v4 5/6] UefiCpuPkg/CpuExceptionHandlerLib: Ensure valid frame/stack pointers Thread-Index: AQHTgF9OsLpC1Xr6UUqXFQhrBGHBE6Ni7s9Q Date: Thu, 4 Jan 2018 01:07:12 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503AA6F907@shsmsx102.ccr.corp.intel.com> References: <836f7f2205e91c16d7c427c5b6e127f4a4dfa62e.1514517573.git.paulo@paulo.ac> In-Reply-To: <836f7f2205e91c16d7c427c5b6e127f4a4dfa62e.1514517573.git.paulo@paulo.ac> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNjQ3ZmQzMTEtZmQ1YS00ZDY5LWJkMGYtZGNiNzE2MzVlYjMwIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjIuNS4xOCIsIlRydXN0ZWRMYWJlbEhhc2giOiJ0NER2V0RNenRvRlBSNktodWN0UGYzM0dyUFwvXC9GTHJ5REx5OVlVbGRpbHB4VHhvNTlhSXZNbTVTR0NaVGduS3kifQ== x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [RFC v4 5/6] UefiCpuPkg/CpuExceptionHandlerLib: Ensure valid frame/stack pointers X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 04 Jan 2018 01:02:51 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Some suggestion: 1) Would you please use meaning definition for BIT2? if ((SegmentSelector & BIT2) =3D=3D 0) { 2) Can we just use (SegmentSelector & ~0x7) for below? ((SegmentSelector >> 3) * 8) 3) Below calculation seems wrong. Should it be: SegDescLimitInBytes =3D (UI= NTN)SegDescLimit * SIZE_4KB + (SIZE_4KB - 1) ? if (SegmentDescriptor->Bits.G =3D=3D 1) { SegDescLimitInBytes =3D (UINTN)SegDescLimit * SIZE_4KB; Thank you Yao Jiewen > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Pa= ulo > Alcantara > Sent: Friday, December 29, 2017 12:40 PM > To: edk2-devel@lists.01.org > Cc: Laszlo Ersek ; Dong, Eric > Subject: [edk2] [RFC v4 5/6] UefiCpuPkg/CpuExceptionHandlerLib: Ensure va= lid > frame/stack pointers >=20 > Validate all possible memory dereferences during stack traces in IA32 > and X64 CPU exceptions. >=20 > Contributed-under: TianoCore Contribution Agreement 1.1 > Cc: Eric Dong > Cc: Laszlo Ersek > Requested-by: Brian Johnson > Requested-by: Jiewen Yao > Signed-off-by: Paulo Alcantara > --- > UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c | > 143 +++++++++++++++++++- > UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c | > 75 +++++++++- > 2 files changed, 210 insertions(+), 8 deletions(-) >=20 > diff --git > a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c > b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c > index 25e02fbbc1..9b52d4f6d2 100644 > --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler= .c > +++ > b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c > @@ -398,6 +398,96 @@ DumpCpuContext ( > ); > } >=20 > +/** > + Check if a logical address is valid. > + > + @param[in] SystemContext Pointer to EFI_SYSTEM_CONTEXT. > + @param[in] SegmentSelector Segment selector. > + @param[in] Offset Offset or logical address. > +**/ > +STATIC > +BOOLEAN > +IsLogicalAddressValid ( > + IN EFI_SYSTEM_CONTEXT SystemContext, > + IN UINT16 SegmentSelector, > + IN UINTN Offset > + ) > +{ > + IA32_SEGMENT_DESCRIPTOR *SegmentDescriptor; > + UINT32 SegDescBase; > + UINT32 SegDescLimit; > + UINTN SegDescLimitInBytes; > + > + // > + // Check for valid input parameters > + // > + if (SegmentSelector =3D=3D 0 || Offset =3D=3D 0) { > + return FALSE; > + } > + > + // > + // Check whether to look for a segment descriptor in GDT or LDT table > + // > + if ((SegmentSelector & BIT2) =3D=3D 0) { > + // > + // Get segment descriptor from GDT table > + // > + SegmentDescriptor =3D > + (IA32_SEGMENT_DESCRIPTOR *)( > + (UINTN)SystemContext.SystemContextIa32->Gdtr[0] + > + ((SegmentSelector >> 3) * 8) > + ); > + } else { > + // > + // Get segment descriptor from LDT table > + // > + SegmentDescriptor =3D > + (IA32_SEGMENT_DESCRIPTOR *)( > + (UINTN)SystemContext.SystemContextIa32->Ldtr + > + ((SegmentSelector >> 3) * 8) > + ); > + } > + > + // > + // Get segment descriptor's base address > + // > + SegDescBase =3D SegmentDescriptor->Bits.BaseLow | > + (SegmentDescriptor->Bits.BaseMid << 16) | > + (SegmentDescriptor->Bits.BaseHigh << 24); > + > + // > + // Get segment descriptor's limit > + // > + SegDescLimit =3D SegmentDescriptor->Bits.LimitLow | > + (SegmentDescriptor->Bits.LimitHigh << 16); > + > + // > + // Calculate segment descriptor's limit in bytes > + // > + if (SegmentDescriptor->Bits.G =3D=3D 1) { > + SegDescLimitInBytes =3D (UINTN)SegDescLimit * SIZE_4KB; > + } else { > + SegDescLimitInBytes =3D SegDescLimit; > + } > + > + // > + // Make sure to not access beyond a segment limit boundary > + // > + if (Offset + SegDescBase > SegDescLimitInBytes) { > + return FALSE; > + } > + > + // > + // Check if the translated logical address (or linear address) is vali= d > + // > + return IsLinearAddressValid ( > + SystemContext.SystemContextIa32->Cr0, > + SystemContext.SystemContextIa32->Cr3, > + SystemContext.SystemContextIa32->Cr4, > + Offset + SegDescBase > + ); > +} > + > /** > Dump stack trace. >=20 > @@ -459,6 +549,20 @@ DumpStackTrace ( > InternalPrintMessage ("\nCall trace:\n"); >=20 > for (;;) { > + // > + // Check for valid frame pointer > + // > + if (!IsLogicalAddressValid (SystemContext, > + SystemContext.SystemContextIa32->Ss, > + (UINTN)Ebp + 4) || > + !IsLogicalAddressValid (SystemContext, > + SystemContext.SystemContextIa32->Ss, > + (UINTN)Ebp)) { > + InternalPrintMessage ("%a: attempted to dereference an invalid fra= me " > + "pointer at 0x%08x\n", __FUNCTION__, Ebp); > + break; > + } > + > // > // Print stack frame in the following format: > // > @@ -588,6 +692,16 @@ DumpImageModuleNames ( > // Walk through call stack and find next module names > // > for (;;) { > + if (!IsLogicalAddressValid (SystemContext, > + SystemContext.SystemContextIa32->Ss, > + (UINTN)Ebp) || > + !IsLogicalAddressValid (SystemContext, > + SystemContext.SystemContextIa32->Ss, > + (UINTN)Ebp + 4)) { > + InternalPrintMessage ("%a: attempted to dereference an invalid fra= me " > + "pointer at 0x%08x\n", __FUNCTION__, Ebp); > + } > + > // > // Set EIP with return address from current stack frame > // > @@ -651,16 +765,23 @@ DumpImageModuleNames ( > /** > Dump stack contents. >=20 > - @param[in] CurrentEsp Current stack pointer address. > + @param[in] SystemContext Pointer to EFI_SYSTEM_CONTEXT. > @param[in] UnwoundStacksCount Count of unwound stack frames. > **/ > STATIC > VOID > DumpStackContents ( > - IN UINT32 CurrentEsp, > - IN INTN UnwoundStacksCount > + IN EFI_SYSTEM_CONTEXT SystemContext, > + IN INTN UnwoundStacksCount > ) > { > + UINT32 CurrentEsp; > + > + // > + // Get current stack pointer > + // > + CurrentEsp =3D SystemContext.SystemContextIa32->Esp; > + > // > // Check for proper stack alignment > // > @@ -674,6 +795,20 @@ DumpStackContents ( > // > InternalPrintMessage ("\nStack dump:\n"); > while (UnwoundStacksCount-- > 0) { > + // > + // Check for a valid stack pointer address > + // > + if (!IsLogicalAddressValid (SystemContext, > + SystemContext.SystemContextIa32->Ss, > + (UINTN)CurrentEsp) || > + !IsLogicalAddressValid (SystemContext, > + SystemContext.SystemContextIa32->Ss, > + (UINTN)CurrentEsp + 4)) { > + InternalPrintMessage ("%a: attempted to dereference an invalid sta= ck " > + "pointer at 0x%08x\n", __FUNCTION__, > CurrentEsp); > + break; > + } > + > InternalPrintMessage ( > "0x%08x: %08x %08x\n", > CurrentEsp, > @@ -720,5 +855,5 @@ DumpImageAndCpuContent ( > // > // Dump stack contents > // > - DumpStackContents (SystemContext.SystemContextIa32->Esp, > UnwoundStacksCount); > + DumpStackContents (SystemContext, UnwoundStacksCount); > } > diff --git > a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c > b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c > index d3a3878b3d..8067c34122 100644 > --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.= c > +++ > b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c > @@ -401,16 +401,26 @@ DumpCpuContext ( > /** > Dump stack contents. >=20 > - @param[in] CurrentRsp Current stack pointer address. > + @param[in] SystemContext Pointer to EFI_SYSTEM_CONTEXT. > @param[in] UnwoundStacksCount Count of unwound stack frames. > **/ > STATIC > VOID > DumpStackContents ( > - IN UINT64 CurrentRsp, > - IN INTN UnwoundStacksCount > + IN EFI_SYSTEM_CONTEXT SystemContext, > + IN INTN UnwoundStacksCount > ) > { > + UINT64 CurrentRsp; > + UINTN Cr0; > + UINTN Cr3; > + UINTN Cr4; > + > + // > + // Get current stack pointer > + // > + CurrentRsp =3D SystemContext.SystemContextX64->Rsp; > + > // > // Check for proper stack pointer alignment > // > @@ -419,11 +429,28 @@ DumpStackContents ( > return; > } >=20 > + // > + // Get system control registers > + // > + Cr0 =3D SystemContext.SystemContextX64->Cr0; > + Cr3 =3D SystemContext.SystemContextX64->Cr3; > + Cr4 =3D SystemContext.SystemContextX64->Cr4; > + > // > // Dump out stack contents > // > InternalPrintMessage ("\nStack dump:\n"); > while (UnwoundStacksCount-- > 0) { > + // > + // Check for a valid stack pointer address > + // > + if (!IsLinearAddressValid (Cr0, Cr3, Cr4, (UINTN)CurrentRsp) || > + !IsLinearAddressValid (Cr0, Cr3, Cr4, (UINTN)CurrentRsp + 8)) { > + InternalPrintMessage ("%a: attempted to dereference an invalid sta= ck " > + "pointer at 0x%016lx\n", __FUNCTION__, > CurrentRsp); > + break; > + } > + > InternalPrintMessage ( > "0x%016lx: %016lx %016lx\n", > CurrentRsp, > @@ -457,6 +484,9 @@ DumpImageModuleNames ( > CHAR8 *PdbFileName; > UINT64 Rbp; > UINTN LastImageBase; > + UINTN Cr0; > + UINTN Cr3; > + UINTN Cr4; >=20 > // > // Set current RIP address > @@ -516,10 +546,27 @@ DumpImageModuleNames ( > InternalPrintMessage ("%a\n", PdbAbsoluteFilePath); > } >=20 > + // > + // Get system control registers > + // > + Cr0 =3D SystemContext.SystemContextX64->Cr0; > + Cr3 =3D SystemContext.SystemContextX64->Cr3; > + Cr4 =3D SystemContext.SystemContextX64->Cr4; > + > // > // Walk through call stack and find next module names > // > for (;;) { > + // > + // Check for a valid frame pointer > + // > + if (!IsLinearAddressValid (Cr0, Cr3, Cr4, (UINTN)Rbp + 8) || > + !IsLinearAddressValid (Cr0, Cr3, Cr4, (UINTN)Rbp)) { > + InternalPrintMessage ("%a: attempted to dereference an invalid fra= me " > + "pointer at 0x%016lx\n", __FUNCTION__, > Rbp); > + break; > + } > + > // > // Set RIP with return address from current stack frame > // > @@ -604,6 +651,9 @@ DumpStackTrace ( > UINT64 Rbp; > UINTN ImageBase; > CHAR8 *PdbFileName; > + UINTN Cr0; > + UINTN Cr3; > + UINTN Cr4; >=20 > // > // Set current RIP address > @@ -634,12 +684,29 @@ DumpStackTrace ( > // > *UnwoundStacksCount =3D 1; >=20 > + // > + // Get system control registers > + // > + Cr0 =3D SystemContext.SystemContextX64->Cr0; > + Cr3 =3D SystemContext.SystemContextX64->Cr3; > + Cr4 =3D SystemContext.SystemContextX64->Cr4; > + > // > // Print out back trace > // > InternalPrintMessage ("\nCall trace:\n"); >=20 > for (;;) { > + // > + // Check for valid frame pointer > + // > + if (!IsLinearAddressValid (Cr0, Cr3, Cr4, (UINTN)Rbp + 8) || > + !IsLinearAddressValid (Cr0, Cr3, Cr4, (UINTN)Rbp)) { > + InternalPrintMessage ("%a: attempted to dereference an invalid fra= me " > + "pointer at 0x%016lx\n", __FUNCTION__, > Rbp); > + break; > + } > + > // > // Print stack frame in the following format: > // > @@ -727,5 +794,5 @@ DumpImageAndCpuContent ( > // > // Dump stack contents > // > - DumpStackContents (SystemContext.SystemContextX64->Rsp, > UnwoundStacksCount); > + DumpStackContents (SystemContext, UnwoundStacksCount); > } > -- > 2.14.3 >=20 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel