From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=34.238.86.106; helo=mail.paulo.ac; envelope-from=paulo@paulo.ac; receiver=edk2-devel@lists.01.org Received: from mail.paulo.ac (mail.paulo.ac [34.238.86.106]) (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 C9E6F21CB2E3D for ; Wed, 3 Jan 2018 06:43:14 -0800 (PST) Received: from localhost (localhost [127.0.0.1]) by mail.paulo.ac (Postfix) with ESMTP id 70985C790F7; Wed, 3 Jan 2018 14:48:17 +0000 (UTC) X-Virus-Scanned: amavisd-new at paulo.ac X-Spam-Flag: NO X-Spam-Score: -1.099 X-Spam-Level: X-Spam-Status: No, score=-1.099 tagged_above=-999 required=6.31 tests=[ALL_TRUSTED=-1, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no Authentication-Results: mail.paulo.ac (amavisd-new); dkim=pass (1024-bit key) header.d=paulo.ac Received: from mail.paulo.ac ([127.0.0.1]) by localhost (mail.paulo.ac [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id eBMp50q7LMqR; Wed, 3 Jan 2018 14:48:13 +0000 (UTC) Received: from [10.10.25.209] (unknown [15.65.243.10]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.paulo.ac (Postfix) with ESMTPSA id 01E6FC78F5B; Wed, 3 Jan 2018 14:48:11 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 mail.paulo.ac 01E6FC78F5B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=paulo.ac; s=default; t=1514990893; bh=zwYm2N1y/GnLsPE2C2HmO9AC/gQORToPdM5VLue/EPA=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=n2zjexrttEfRoUeh3y2c/6XEDQZ3qqRMcXZujCLGELH/ByA8LWl8rNwonmp6lw/V8 PdjNvqoCszO7ys6gSOcTboHCuX/lTZgiFC8kF+x/HO5fn4Puid96MNOJxP/HZHqycQ NJaSU2oeyT5+gI+zFAXyMOXH6vqAJDv8USl1jzqY= To: Fan Jeff , "edk2-devel@lists.01.org" Cc: Laszlo Ersek , Eric Dong References: <836f7f2205e91c16d7c427c5b6e127f4a4dfa62e.1514517573.git.paulo@paulo.ac> From: Paulo Alcantara Message-ID: <517a9298-db1a-2410-4cad-afd60541ec65@paulo.ac> Date: Wed, 3 Jan 2018 12:48:09 -0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: Subject: =?UTF-8?B?UmU6IOetlOWkjTogW1JGQyB2NCA1LzZdIFVlZmlDcHVQa2cvQ3B1RXhjZXB0aW9uSGFuZGxlckxpYjogRW5zdXJlIHZhbGlkIGZyYW1lL3N0YWNrIHBvaW50ZXJz?= 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: Wed, 03 Jan 2018 14:43:15 -0000 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 1/3/2018 6:45 AM, Fan Jeff wrote: > Paulo, > > +    if (!IsLogicalAddressValid (SystemContext, > +                                SystemContext.SystemContextIa32->Ss, > +                                (UINTN)Ebp) || > +        !IsLogicalAddressValid (SystemContext, > +                                SystemContext.SystemContextIa32->Ss, > +                                (UINTN)Ebp + 4)) { > > I don’t understand why you check both ebp and ebp+4, I think it’s enough > to only check EBP (saved stack pointer address) Isn't it possible that EBP + 4 might potentially point to another page frame? If not, then I will drop it out in v5. Thanks Paulo > > Jeff > > *发件人: *Paulo Alcantara > *发送时间: *2017年12月29日12:41 > *收件人: *edk2-devel@lists.01.org > *抄送: *Laszlo Ersek ; Eric Dong > > *主题: *[edk2] [RFC v4 5/6] UefiCpuPkg/CpuExceptionHandlerLib: Ensure > valid frame/stack pointers > > Validate all possible memory dereferences during stack traces in IA32 > and X64 CPU exceptions. > > 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(-) > > 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 ( >      ); >  } > > +/** > +  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 == 0 || Offset == 0) { > +    return FALSE; > +  } > + > +  // > +  // Check whether to look for a segment descriptor in GDT or LDT table > +  // > +  if ((SegmentSelector & BIT2) == 0) { > +    // > +    // Get segment descriptor from GDT table > +    // > +    SegmentDescriptor = > +      (IA32_SEGMENT_DESCRIPTOR *)( > +        (UINTN)SystemContext.SystemContextIa32->Gdtr[0] + > +        ((SegmentSelector >> 3) * 8) > +        ); > +  } else { > +    // > +    // Get segment descriptor from LDT table > +    // > +    SegmentDescriptor = > +      (IA32_SEGMENT_DESCRIPTOR *)( > +        (UINTN)SystemContext.SystemContextIa32->Ldtr + > +        ((SegmentSelector >> 3) * 8) > +        ); > +  } > + > +  // > +  // Get segment descriptor's base address > +  // > +  SegDescBase = SegmentDescriptor->Bits.BaseLow | > +    (SegmentDescriptor->Bits.BaseMid << 16) | > +    (SegmentDescriptor->Bits.BaseHigh << 24); > + > +  // > +  // Get segment descriptor's limit > +  // > +  SegDescLimit = SegmentDescriptor->Bits.LimitLow | > +    (SegmentDescriptor->Bits.LimitHigh << 16); > + > +  // > +  // Calculate segment descriptor's limit in bytes > +  // > +  if (SegmentDescriptor->Bits.G == 1) { > +    SegDescLimitInBytes = (UINTN)SegDescLimit * SIZE_4KB; > +  } else { > +    SegDescLimitInBytes = 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 valid > +  // > +  return IsLinearAddressValid ( > +    SystemContext.SystemContextIa32->Cr0, > +    SystemContext.SystemContextIa32->Cr3, > +    SystemContext.SystemContextIa32->Cr4, > +    Offset + SegDescBase > +    ); > +} > + >  /** >    Dump stack trace. > > @@ -459,6 +549,20 @@ DumpStackTrace ( >    InternalPrintMessage ("\nCall trace:\n"); > >    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 > frame " > +                            "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 > frame " > +                            "pointer at 0x%08x\n", __FUNCTION__, Ebp); > +    } > + >      // >      // Set EIP with return address from current stack frame >      // > @@ -651,16 +765,23 @@ DumpImageModuleNames ( >  /** >    Dump stack contents. > > -  @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 = 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 > stack " > +                            "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. > > -  @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 = SystemContext.SystemContextX64->Rsp; > + >    // >    // Check for proper stack pointer alignment >    // > @@ -419,11 +429,28 @@ DumpStackContents ( >      return; >    } > > +  // > +  // Get system control registers > +  // > +  Cr0 = SystemContext.SystemContextX64->Cr0; > +  Cr3 = SystemContext.SystemContextX64->Cr3; > +  Cr4 = 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 > stack " > +                            "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; > >    // >    // Set current RIP address > @@ -516,10 +546,27 @@ DumpImageModuleNames ( >      InternalPrintMessage ("%a\n", PdbAbsoluteFilePath); >    } > > +  // > +  // Get system control registers > +  // > +  Cr0 = SystemContext.SystemContextX64->Cr0; > +  Cr3 = SystemContext.SystemContextX64->Cr3; > +  Cr4 = 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 > frame " > +                            "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; > >    // >    // Set current RIP address > @@ -634,12 +684,29 @@ DumpStackTrace ( >    // >    *UnwoundStacksCount = 1; > > +  // > +  // Get system control registers > +  // > +  Cr0 = SystemContext.SystemContextX64->Cr0; > +  Cr3 = SystemContext.SystemContextX64->Cr3; > +  Cr4 = SystemContext.SystemContextX64->Cr4; > + >    // >    // Print out back trace >    // >    InternalPrintMessage ("\nCall trace:\n"); > >    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 > frame " > +                            "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 > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel >