From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:400c:c09::230; helo=mail-wm0-x230.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wm0-x230.google.com (mail-wm0-x230.google.com [IPv6:2a00:1450:400c:c09::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 964342097F557 for ; Tue, 3 Jul 2018 07:35:16 -0700 (PDT) Received: by mail-wm0-x230.google.com with SMTP id p11-v6so2531745wmc.4 for ; Tue, 03 Jul 2018 07:35:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=8WwiZlgzQRNW9STM49/nhkLX+O27IPXY/V+I2hZK67I=; b=dY7pNghay6K65XjOaICuXKZroQG1kCPHSxrAob0pvw5RtmDo55N/Nv1AU5uXDBKeey 2OP4H5DjxTlz23cqQEGOuuaIPdigVbZsH2p9LwUx+pcS+Wn+9F0sG91sDdbOypek2ZE6 2Ax04HXIZefscmmMr3BMsur4G62MxB+SFhqWI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=8WwiZlgzQRNW9STM49/nhkLX+O27IPXY/V+I2hZK67I=; b=Ek4GkEUwb7UtUsNFTZLcyyXrpd+EgFQuUEca7eEeCJKkOOkQ61IRZdY/HHOf+0TEkq AQuMxCrK/nWq80uxJxYcKM1VnVc7fZ+o8Wit4coTCGkkgcKEhURy7vdetz0PvjL4P1eP 1krEAsg4+Ztd6/FtIfwnfNxb9Bus8VtSW5obaWfEzu9lEpmOtn9SMHtA0wOeWM7CfPws xLbs11SPOGLtn+8WXYIa9D2Jd8kGpDgDq62DIkCi3P03zIy92qWKGzKed/Wt1mC14EXM kQS15CDnpAmrz2UvZYuaFiaZ5XeoMQdrV2sOOWSVIlQxDPbEQV+H0yZwPShgWkci4Lr5 EwqQ== X-Gm-Message-State: APt69E39JfQq1Hadxp24gzBB10MBxXqsaxvSM1HTyCbHNEDL6rVx8H/t LVeJT8VYIUmhejakfMbAaSvkew== X-Google-Smtp-Source: AAOMgpfLFyznRlxS+TPIpBA9aaCvLS7dQm1MmKV286gMAV3292WnZ3iV5gicbRpWayU+t5e5zfD7Vw== X-Received: by 2002:a1c:ea9b:: with SMTP id g27-v6mr5005153wmi.152.1530628514757; Tue, 03 Jul 2018 07:35:14 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id e13-v6sm2813616wrd.9.2018.07.03.07.35.13 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 03 Jul 2018 07:35:13 -0700 (PDT) Date: Tue, 3 Jul 2018 15:35:12 +0100 From: Leif Lindholm To: Supreeth Venkatesh Cc: edk2-devel@lists.01.org, Achin Gupta , Ard Biesheuvel Message-ID: <20180703143512.v5yvdxc7whqhnerg@bivouac.eciton.net> References: <1530611715-9819-1-git-send-email-supreeth.venkatesh@arm.com> <1530611715-9819-7-git-send-email-supreeth.venkatesh@arm.com> MIME-Version: 1.0 In-Reply-To: <1530611715-9819-7-git-send-email-supreeth.venkatesh@arm.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH 6/6] ArmPkg: Extra action to update permissions for S-ELO MM Image. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 03 Jul 2018 14:35:17 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Tue, Jul 03, 2018 at 03:25:15PM +0530, Supreeth Venkatesh wrote: > 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: Achin Gupta > Signed-off-by: Supreeth Venkatesh > Cc: Leif Lindholm > Cc: Ard Biesheuvel > --- > .../DebugPeCoffExtraActionLib.c | 185 +++++++++++++++++++-- > .../DebugPeCoffExtraActionLib.inf | 7 + > 2 files changed, 181 insertions(+), 11 deletions(-) > > diff --git a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c > index f298e58..c87aaf0 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,23 +234,29 @@ PeCoffLoaderRelocateImageExtraAction ( > CHAR8 Temp[512]; > #endif > > + if (PcdGetBool(PcdStandaloneMmEnable) == TRUE) > + { '{' at end of previous line. > + UpdatePeCoffPermissions (ImageContext, ArmClearMemoryRegionNoExec, > + ArmSetMemoryRegionReadOnly); > + } > + > if (ImageContext->PdbPointer) { > #ifdef __CC_ARM > #if (__ARMCC_VERSION < 500000) > // Print out the command for the RVD debugger to load symbols for this image > - DEBUG ((EFI_D_LOAD | EFI_D_INFO, "load /a /ni /np %a &0x%p\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders))); > + DEBUG ((DEBUG_LOAD | DEBUG_INFO, "load /a /ni /np %a &0x%p\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders))); This does not look like a functional change. Please do style changes as a separate patch or not at all. Applies many times below. > #else > // Print out the command for the DS-5 to load symbols for this image > - DEBUG ((EFI_D_LOAD | EFI_D_INFO, "add-symbol-file %a 0x%p\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders))); > + DEBUG ((DEBUG_LOAD | DEBUG_INFO, "add-symbol-file %a 0x%p\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders))); > #endif > #elif __GNUC__ > // This may not work correctly if you generate PE/COFF directlyas then the Offset would not be required > - DEBUG ((EFI_D_LOAD | EFI_D_INFO, "add-symbol-file %a 0x%p\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders))); > + DEBUG ((DEBUG_LOAD | DEBUG_INFO, "add-symbol-file %a 0x%p\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders))); > #else > - DEBUG ((EFI_D_LOAD | EFI_D_INFO, "Loading driver at 0x%11p EntryPoint=0x%11p\n", (VOID *)(UINTN) ImageContext->ImageAddress, FUNCTION_ENTRY_POINT (ImageContext->EntryPoint))); > + DEBUG ((DEBUG_LOAD | DEBUG_INFO, "Loading driver at 0x%11p EntryPoint=0x%11p\n", (VOID *)(UINTN) ImageContext->ImageAddress, FUNCTION_ENTRY_POINT (ImageContext->EntryPoint))); > #endif > } else { > - DEBUG ((EFI_D_LOAD | EFI_D_INFO, "Loading driver at 0x%11p EntryPoint=0x%11p\n", (VOID *)(UINTN) ImageContext->ImageAddress, FUNCTION_ENTRY_POINT (ImageContext->EntryPoint))); > + DEBUG ((DEBUG_LOAD | DEBUG_INFO, "Loading driver at 0x%11p EntryPoint=0x%11p\n", (VOID *)(UINTN) ImageContext->ImageAddress, FUNCTION_ENTRY_POINT (ImageContext->EntryPoint))); > } > } > > @@ -125,17 +282,23 @@ PeCoffLoaderUnloadImageExtraAction ( > CHAR8 Temp[512]; > #endif > > + if (PcdGetBool(PcdStandaloneMmEnable) == TRUE) > + { '{' on preceding line. / Leif > + UpdatePeCoffPermissions (ImageContext, ArmSetMemoryRegionNoExec, > + ArmClearMemoryRegionReadOnly); > + } > + > if (ImageContext->PdbPointer) { > #ifdef __CC_ARM > // Print out the command for the RVD debugger to load symbols for this image > - DEBUG ((EFI_D_ERROR, "unload symbols_only %a\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)))); > + DEBUG ((DEBUG_ERROR, "unload symbols_only %a\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)))); > #elif __GNUC__ > // This may not work correctly if you generate PE/COFF directlyas then the Offset would not be required > - DEBUG ((EFI_D_ERROR, "remove-symbol-file %a 0x%08x\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders))); > + DEBUG ((DEBUG_ERROR, "remove-symbol-file %a 0x%08x\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders))); > #else > - DEBUG ((EFI_D_ERROR, "Unloading %a\n", ImageContext->PdbPointer)); > + DEBUG ((DEBUG_ERROR, "Unloading %a\n", ImageContext->PdbPointer)); > #endif > } else { > - DEBUG ((EFI_D_ERROR, "Unloading driver at 0x%11p\n", (VOID *)(UINTN) ImageContext->ImageAddress)); > + DEBUG ((DEBUG_ERROR, "Unloading driver at 0x%11p\n", (VOID *)(UINTN) ImageContext->ImageAddress)); > } > } > diff --git a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.inf b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.inf > index c1f717e..38bf399 100644 > --- a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.inf > +++ b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.inf > @@ -33,7 +33,14 @@ > DebugPeCoffExtraActionLib.c > > [Packages] > + ArmPkg/ArmPkg.dec > MdePkg/MdePkg.dec > + StandaloneMmPkg/StandaloneMmPkg.dec > + > +[FeaturePcd] > + gStandaloneMmPkgTokenSpaceGuid.PcdStandaloneMmEnable > > [LibraryClasses] > + ArmMmuLib > DebugLib > + PcdLib > -- > 2.7.4 >