From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4864:20::142; helo=mail-it1-x142.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it1-x142.google.com (mail-it1-x142.google.com [IPv6:2607:f8b0:4864:20::142]) (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 D9E1A21163272 for ; Wed, 24 Oct 2018 04:05:23 -0700 (PDT) Received: by mail-it1-x142.google.com with SMTP id h6-v6so14663959ith.0 for ; Wed, 24 Oct 2018 04:05:23 -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=sokFkaD2g8Mm5D7m4zPUV7QGGoMVyGwwa0GzORcerYA=; b=EeHzhx44tfLTwecHMSthhHzIF2NGuWqoJKQfNyxNYY7cN7YbKpKfELZNtoPHGrdc9E zyH1tvGhLcLHCjS5LbqtjuC3Fi5MdiwQG+VRE7TbdOu8mfJM/sAGR57Hkqht3SPYW+D2 PVfmTA0mIy9Lx94I7A7dTnH6HIGl0JCjqJGW0= 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=sokFkaD2g8Mm5D7m4zPUV7QGGoMVyGwwa0GzORcerYA=; b=pabAqODiffv7UVNHOujcBvc4w34vtVw3ApYdcxz7ZVV2qPvq3Bpj7KmT32tO1wnfKG BcJVclEJSXxv8EmySeBIZPsFgEHXQ7Hhvexrf+T7WszBjm759+B6myWJYU96sB35O/+h unTlEus/coZCmJHmp4gTH6cnIl5SpRjOoaJIy0XhoWLHNxWGAChQ03gr/ik76X+65Qyx XmnElJASql5mvpZi6O5FIGMd4EmM8HimW2oJnDxDi6ePllbF8Ka3KRmqi22oPq72eZzn Nrpuv368PLDxpTJQzK3OOSRyW9jQpWOziT1ApO3+tsdcuhMZuPJ1zuDHVyetFM646rY6 vQ/g== X-Gm-Message-State: AGRZ1gI5pIJjujQas4J+Z3ZUkoRBFLvIwH12l8a0TmPiwWIN4zb4SxHF g1Z58EMrFRDlWrNBSngY8pksVscP9OVZLsqf4OytFA== X-Google-Smtp-Source: AJdET5dSnkCwSjpxIOPhaWkGtSxzNcxRUPha13XBd/7iRCR3MZbUUhFuu/0jUteiz/guQpwctSdp5DBKCBUZa0LpZ80= X-Received: by 2002:a05:660c:383:: with SMTP id x3mr1012185itj.121.1540379122520; Wed, 24 Oct 2018 04:05:22 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a6b:5910:0:0:0:0:0 with HTTP; Wed, 24 Oct 2018 04:05:22 -0700 (PDT) In-Reply-To: <20181024082212.GD4897@e104320-lin> References: <1532090300-5250-1-git-send-email-sughosh.ganu@arm.com> <1532090300-5250-8-git-send-email-sughosh.ganu@arm.com> <1532367194.3302.36.camel@arm.com> <20180821065047.GA17216@arm.com> <20181024082212.GD4897@e104320-lin> From: Ard Biesheuvel Date: Wed, 24 Oct 2018 08:05:22 -0300 Message-ID: To: Achin Gupta Cc: Sughosh Ganu , "edk2-devel@lists.01.org" , nd 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.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 24 Oct 2018 11:05:24 -0000 Content-Type: text/plain; charset="UTF-8" On 24 October 2018 at 05:22, Achin Gupta wrote: > Hi Ard, > > Please see CIL.. > FYI I will be on leave until 5th of November, so I will get to this once I get back. -- Ard. > On Fri, Aug 24, 2018 at 03:55:29PM +0100, Ard Biesheuvel wrote: >> On 21 August 2018 at 07:50, Sughosh Ganu wrote: >> > hi Ard, >> > >> > On Tue July 23, 2018 at 11:03PM +0530, Supreeth Venkatesh wrote: >> >> >> >> On Sat, 2018-07-21 at 20:06 +0900, Ard Biesheuvel wrote: >> >> > 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. >> >> > >> >> Apologies if you have missed it. But I am pretty sure it was part of >> >> earlier large patch-set on which you and leif were copied, as it was >> >> part of ArmPkg. >> >> > >> >> > 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) >> >> I think we need a little bit more details on what is your suggestion? >> >> >> >> A little bit background here: This code runs in S-EL0 and Request gets >> >> sent to secure world SPM to ensure that the region permissions are >> >> updated correctly via the "ArmMmuStandaloneMmCoreLib" SVC - >> >> ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES_AARCH64. >> >> >> >> DebugPeCoffExtraActionLib is just used to extract image region >> >> information, but the region permission >> >> update request is sent to secure world for validation. >> >> >> >> With the above explanation, can you provide an insight into what was >> >> your thinking? >> >> Do you want us to create a separate library and call it >> >> as PeCoffExtraActionLib to avoid the "Debug" word though it is a hook >> >> to PeCoffExtraActionLib in MdePkg or do we want to create this library >> >> in a separate package (may be in MdePkg?) or something totally >> >> different. >> > >> > Supreeth had replied to your comments on the patch. Can you please >> > check this. If you feel that this needs to be implemented differently, >> > can you please suggest it to us. Thanks. >> > >> >> My point is that such a fundamental action that needs to occur while >> loading the PE/COFF image should not be hooked into the loader this >> way. > > Based upon our discussion at the Linaro Connect, we investigated leveraging the > DXE Image Protection support [1] in Standalone MM (StMM). Amongst other > challenges, there is a chicken and egg problem. I will try and explain. > > DXE Memory protection has dependencies that cannot be fulfilled in StMM. A > non-exhaustive list is: > > 1. Dependency on CPU_ARCH protocol > 2. Dependency on Loaded Image patch protocol > 3. Dependency on Boot services > > There is an inherent assumption that this support will never be used in > SMM. Furthermore, in StMM, permissions are changed when the StMM drivers are > first dispatched. A dependency on a driver to change the permissions is the > chicken and egg. So we need a library. > > One option is to introduce a memory protection library in StMM i.e. a library > interface like StandaloneMmImageProtect(). This function will be called from > generic code after the PE-COFF loader has loaded and relocated the StMM driver > image. However, this support is not required on x86. They will have to include a > NULL library implementation. This would be in addition to the NULL > PeCoffExtraActionLib they already include through MdePkg.dsc. > > I am hesitant to take this approach in the absence of a requirement on x86. At > the same time, the current approach of leveraging the DebugPeCoffExtraActionLib > in ArmPkg does not make sense either. > > IMO, the better approach would be to add a AArch64 specific > StandaloneMmPeCoffExtraActionLib in the StandaloneMmPkg. Memory protection will > be implemented in the relocation hook. There will be no impact on x86 or the > ArmPkg. If in future there is a requirement to support this feature on x86 as > well, then a separate library could be implemented. > > Please let us know if this sounds reasonable to you. Sughosh will be posting the > patches with this approach in a bit to aid the discussion. > > Cheers, > Achin > > [1] MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c > >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel