From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::230; helo=mail-it0-x230.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x230.google.com (mail-it0-x230.google.com [IPv6:2607:f8b0:4001:c0b::230]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id AF2E8210BFF5F for ; Sat, 21 Jul 2018 04:06:08 -0700 (PDT) Received: by mail-it0-x230.google.com with SMTP id y124-v6so7399058itc.0 for ; Sat, 21 Jul 2018 04:06:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=P8FsJj72buZKa9p5y+NdFUa10YLQz7SIA0Jk96D5AuU=; b=JX7Dsm38zW9BPvO7BS35SRt3ZI3ClNxygT+ccH2hcN/g8n4TvPdMkE7QijSj48nkce zoLUHQsby2mHCUVvE51F+BiXIdDqjZfB48x8RI1Xs6FV1cjzl0gDbIjZocS3nmEahrtg V0DSetJHx021RBbUovKloDvPNnWWyl1/wqDKs= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=P8FsJj72buZKa9p5y+NdFUa10YLQz7SIA0Jk96D5AuU=; b=VuQJqJKhikdcwMwLMSJvKUWaOdIus7T+63jR3BgK7rC0aQfXfcJBRC4BzWH8WJ1FrO gAi272zvH4/nlIKPab56XWJqHF7e1O6iShQMqvJr9GdSig7I920HeJtPa4T+Alf35FEA aHyR0k6uY8jzX99k2eY/5pJ79wKz2IPd388U2/z7aq8gYYLL7rELRsAZ4PoZLG9dYT+R LEoqZwuSMke5QDUu1v4O9OHfgJiL0Cp8hJD0bNzBTixaam1yYK9xQNbXFqVh0WKcn4/+ VzELL1rCLmiXaT580yo+9Uh3GNm2Mx905+4Gdd8fSZ4a+Lwfd8jLtrX3/Of8VGckLyK1 zvXQ== X-Gm-Message-State: AOUpUlE1u0v+fTKgAkD4o5QGErsXFpzPLrBUiAGWP1P80tDTAj7FHBBv aTlTSI6lfKMkK+0NvBGmEGCanU+1uZs7urWCHHtuwg== X-Google-Smtp-Source: AAOMgpcW5+go7hx52GeCHzw6xQTUBIWeoX+06vPMdYkgxnKHNfOqIV16qhQgQ3dAS13YClwUOmaDg2ISX/ok5OKJcws= X-Received: by 2002:a24:610d:: with SMTP id s13-v6mr4751106itc.68.1532171167139; Sat, 21 Jul 2018 04:06:07 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a6b:ac05:0:0:0:0:0 with HTTP; Sat, 21 Jul 2018 04:06:06 -0700 (PDT) In-Reply-To: <1532090300-5250-8-git-send-email-sughosh.ganu@arm.com> References: <1532090300-5250-1-git-send-email-sughosh.ganu@arm.com> <1532090300-5250-8-git-send-email-sughosh.ganu@arm.com> From: Ard Biesheuvel Date: Sat, 21 Jul 2018 20:06:06 +0900 Message-ID: To: Sughosh Ganu Cc: "edk2-devel@lists.01.org" , Leif Lindholm Subject: Re: [PATCH v2 7/7] ArmPkg: Extra action to update permissions for S-ELO MM Image X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 21 Jul 2018 11:06:09 -0000 Content-Type: text/plain; charset="UTF-8" On 20 July 2018 at 21:38, Sughosh Ganu wrote: > From: Achin Gupta > > The Standalone MM drivers runs in S-EL0 in AArch64 on ARM Standard > Platforms and is deployed during SEC phase. The memory allocated to > the Standalone MM drivers should be marked as RO+X. > > During PE/COFF Image section parsing, this patch implements extra > action "UpdatePeCoffPermissions" to request the privileged firmware in > EL3 to update the permissions. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Sughosh Ganu Apologies for bringing this up only now, but I don't think I was ever cc'ed on these patches. We are relying on a debug hook in the PE/COFF loader to ensure that we don't end up with memory that is both writable and executable in the secure world. Do we really think that is a good idea? (I know this code was derived from a proof of concept that I did years ago, but that was just a PoC) > --- > ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.inf | 7 + > ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c | 171 +++++++++++++++++++- > 2 files changed, 176 insertions(+), 2 deletions(-) > > diff --git a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.inf b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.inf > index c1f717e5bda1..38bf3993ae99 100644 > --- a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.inf > +++ b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.inf > @@ -33,7 +33,14 @@ [Sources.common] > DebugPeCoffExtraActionLib.c > > [Packages] > + ArmPkg/ArmPkg.dec > MdePkg/MdePkg.dec > + StandaloneMmPkg/StandaloneMmPkg.dec > + > +[FeaturePcd] > + gStandaloneMmPkgTokenSpaceGuid.PcdStandaloneMmEnable > > [LibraryClasses] > + ArmMmuLib > DebugLib > + PcdLib > diff --git a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c > index f298e58cdfca..8e621de4a87a 100644 > --- a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c > +++ b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c > @@ -15,14 +15,165 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > **/ > > #include > -#include > > +#include > #include > -#include > #include > +#include > +#include > +#include > #include > #include > > +typedef RETURN_STATUS (*REGION_PERMISSION_UPDATE_FUNC) ( > + IN EFI_PHYSICAL_ADDRESS BaseAddress, > + IN UINT64 Length > + ); > + > +STATIC > +RETURN_STATUS > +UpdatePeCoffPermissions ( > + IN CONST PE_COFF_LOADER_IMAGE_CONTEXT *ImageContext, > + IN REGION_PERMISSION_UPDATE_FUNC NoExecUpdater, > + IN REGION_PERMISSION_UPDATE_FUNC ReadOnlyUpdater > + ) > +{ > + RETURN_STATUS Status; > + EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION Hdr; > + EFI_IMAGE_OPTIONAL_HEADER_UNION HdrData; > + UINTN Size; > + UINTN ReadSize; > + UINT32 SectionHeaderOffset; > + UINTN NumberOfSections; > + UINTN Index; > + EFI_IMAGE_SECTION_HEADER SectionHeader; > + PE_COFF_LOADER_IMAGE_CONTEXT TmpContext; > + EFI_PHYSICAL_ADDRESS Base; > + > + // > + // We need to copy ImageContext since PeCoffLoaderGetImageInfo () > + // will mangle the ImageAddress field > + // > + CopyMem (&TmpContext, ImageContext, sizeof (TmpContext)); > + > + if (TmpContext.PeCoffHeaderOffset == 0) { > + Status = PeCoffLoaderGetImageInfo (&TmpContext); > + if (RETURN_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, > + "%a: PeCoffLoaderGetImageInfo () failed (Status = %r)\n", > + __FUNCTION__, Status)); > + return Status; > + } > + } > + > + if (TmpContext.IsTeImage && > + TmpContext.ImageAddress == ImageContext->ImageAddress) { > + DEBUG ((DEBUG_INFO, "%a: ignoring XIP TE image at 0x%lx\n", __FUNCTION__, > + ImageContext->ImageAddress)); > + return RETURN_SUCCESS; > + } > + > + if (TmpContext.SectionAlignment < EFI_PAGE_SIZE) { > + // > + // The sections need to be at least 4 KB aligned, since that is the > + // granularity at which we can tighten permissions. So just clear the > + // noexec permissions on the entire region. > + // > + if (!TmpContext.IsTeImage) { > + DEBUG ((DEBUG_WARN, > + "%a: non-TE Image at 0x%lx has SectionAlignment < 4 KB (%lu)\n", > + __FUNCTION__, ImageContext->ImageAddress, TmpContext.SectionAlignment)); > + } > + Base = ImageContext->ImageAddress & ~(EFI_PAGE_SIZE - 1); > + Size = ImageContext->ImageAddress - Base + ImageContext->ImageSize; > + return NoExecUpdater (Base, ALIGN_VALUE (Size, EFI_PAGE_SIZE)); > + } > + > + // > + // Read the PE/COFF Header. For PE32 (32-bit) this will read in too much > + // data, but that should not hurt anything. Hdr.Pe32->OptionalHeader.Magic > + // determines if this is a PE32 or PE32+ image. The magic is in the same > + // location in both images. > + // > + Hdr.Union = &HdrData; > + Size = sizeof (EFI_IMAGE_OPTIONAL_HEADER_UNION); > + ReadSize = Size; > + Status = TmpContext.ImageRead (TmpContext.Handle, > + TmpContext.PeCoffHeaderOffset, &Size, Hdr.Pe32); > + if (RETURN_ERROR (Status) || (Size != ReadSize)) { > + DEBUG ((DEBUG_ERROR, > + "%a: TmpContext.ImageRead () failed (Status = %r)\n", > + __FUNCTION__, Status)); > + return Status; > + } > + > + ASSERT (Hdr.Pe32->Signature == EFI_IMAGE_NT_SIGNATURE); > + > + SectionHeaderOffset = TmpContext.PeCoffHeaderOffset + sizeof (UINT32) + > + sizeof (EFI_IMAGE_FILE_HEADER); > + NumberOfSections = (UINTN)(Hdr.Pe32->FileHeader.NumberOfSections); > + > + switch (Hdr.Pe32->OptionalHeader.Magic) { > + case EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC: > + SectionHeaderOffset += Hdr.Pe32->FileHeader.SizeOfOptionalHeader; > + break; > + case EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC: > + SectionHeaderOffset += Hdr.Pe32Plus->FileHeader.SizeOfOptionalHeader; > + break; > + default: > + ASSERT (FALSE); > + } > + > + // > + // Iterate over the sections > + // > + for (Index = 0; Index < NumberOfSections; Index++) { > + // > + // Read section header from file > + // > + Size = sizeof (EFI_IMAGE_SECTION_HEADER); > + ReadSize = Size; > + Status = TmpContext.ImageRead (TmpContext.Handle, SectionHeaderOffset, > + &Size, &SectionHeader); > + if (RETURN_ERROR (Status) || (Size != ReadSize)) { > + DEBUG ((DEBUG_ERROR, > + "%a: TmpContext.ImageRead () failed (Status = %r)\n", > + __FUNCTION__, Status)); > + return Status; > + } > + > + Base = TmpContext.ImageAddress + SectionHeader.VirtualAddress; > + > + if ((SectionHeader.Characteristics & EFI_IMAGE_SCN_MEM_EXECUTE) == 0) { > + > + if ((SectionHeader.Characteristics & EFI_IMAGE_SCN_MEM_WRITE) == 0 && > + TmpContext.ImageType != EFI_IMAGE_SUBSYSTEM_EFI_RUNTIME_DRIVER) { > + > + DEBUG ((DEBUG_INFO, > + "%a: Mapping section %d of image at 0x%lx with RO-XN permissions and size 0x%x\n", > + __FUNCTION__, Index, Base, SectionHeader.Misc.VirtualSize)); > + ReadOnlyUpdater (Base, SectionHeader.Misc.VirtualSize); > + } else { > + DEBUG ((DEBUG_WARN, > + "%a: Mapping section %d of image at 0x%lx with RW-XN permissions and size 0x%x\n", > + __FUNCTION__, Index, Base, SectionHeader.Misc.VirtualSize)); > + } > + } else { > + DEBUG ((DEBUG_INFO, > + "%a: Mapping section %d of image at 0x%lx with RO-XN permissions and size 0x%x\n", > + __FUNCTION__, Index, Base, SectionHeader.Misc.VirtualSize)); > + ReadOnlyUpdater (Base, SectionHeader.Misc.VirtualSize); > + > + DEBUG ((DEBUG_INFO, > + "%a: Mapping section %d of image at 0x%lx with RO-X permissions and size 0x%x\n", > + __FUNCTION__, Index, Base, SectionHeader.Misc.VirtualSize)); > + NoExecUpdater (Base, SectionHeader.Misc.VirtualSize); > + } > + > + SectionHeaderOffset += sizeof (EFI_IMAGE_SECTION_HEADER); > + } > + return RETURN_SUCCESS; > +} > > /** > If the build is done on cygwin the paths are cygpaths. > @@ -83,6 +234,14 @@ PeCoffLoaderRelocateImageExtraAction ( > CHAR8 Temp[512]; > #endif > > + if (PcdGetBool(PcdStandaloneMmEnable) == TRUE) { > + UpdatePeCoffPermissions ( > + ImageContext, > + ArmClearMemoryRegionNoExec, > + ArmSetMemoryRegionReadOnly > + ); > + } > + > if (ImageContext->PdbPointer) { > #ifdef __CC_ARM > #if (__ARMCC_VERSION < 500000) > @@ -125,6 +284,14 @@ PeCoffLoaderUnloadImageExtraAction ( > CHAR8 Temp[512]; > #endif > > + if (PcdGetBool(PcdStandaloneMmEnable) == TRUE) { > + UpdatePeCoffPermissions ( > + ImageContext, > + ArmSetMemoryRegionNoExec, > + ArmClearMemoryRegionReadOnly > + ); > + } > + > if (ImageContext->PdbPointer) { > #ifdef __CC_ARM > // Print out the command for the RVD debugger to load symbols for this image > -- > 2.7.4 > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel