From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x22e.google.com (mail-it0-x22e.google.com [IPv6:2607:f8b0:4001:c0b::22e]) (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 88EDA80466 for ; Mon, 20 Mar 2017 12:31:20 -0700 (PDT) Received: by mail-it0-x22e.google.com with SMTP id w124so99795756itb.1 for ; Mon, 20 Mar 2017 12:31:20 -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=HUsEp7rBv//l3cJKP87Xppa65Uj2KM7PXC9Re8cRd0Q=; b=IbdpMmL9N08q9cNvNeo40Pa5fqzBnTR5FD+tTeSVPuS4tXYGtH3uDiF/tH7Bh8DrqR AdwCHWvNK5QtC/DRI8yNzQqh3Z3rQjSMWc9dsRm22ZwXGf5/1Lr7wC2kNZH9iyTin5bk THYomgyDZnbtXSrp/7ERUV1IjPXoQJHB6Jk0Y= 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=HUsEp7rBv//l3cJKP87Xppa65Uj2KM7PXC9Re8cRd0Q=; b=AsVFfv4ezK32+vTQ+jke5aIlejErDJVjZuEZwXR3WL68Z2MiqpQAOJykmXhDRj0oOw w0ui+nNaVgMv4wnlXYodBASdVTIGBy+9onUXWfX0k5/J3j+xWU0MuzMcslAQC2znvB9O eTS5zdnVjuSvOjFiH8WsyTSTLRbMbWa8quFhSWvh3OAuXmlqZRkBtiAOsaXl2laGjDBf rB4Ca33/TAWLwsd1TwFib+QlDzCfgv7w5RxTTbYvp8O0yyJEw+F38AKwBsjbZHuYnwMV C27AmSeFsrasVYOKFUmYX6YBmc2V8GrWvEaQLn0ApvK+k3N7k+t/UJ5vNvgwOdlI22d9 +xKw== X-Gm-Message-State: AFeK/H1w0Rr1rr4h7yAU0w2estCe9Xn6UKyM9NKihVqjErUTt8Qcif7Mvcu4OiNJ6AQbFPBpA3itsaLEtnfsv+Pi X-Received: by 10.107.132.155 with SMTP id o27mr27114678ioi.138.1490038279703; Mon, 20 Mar 2017 12:31:19 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.10.27 with HTTP; Mon, 20 Mar 2017 12:31:19 -0700 (PDT) In-Reply-To: References: <72d13754-5c7d-9734-33a8-bd2dab32b2bb@redhat.com> From: Ard Biesheuvel Date: Mon, 20 Mar 2017 19:31:19 +0000 Message-ID: To: Laszlo Ersek Cc: Michael Zimmermann , edk2-devel-01 Subject: Re: SetMemorySpaceAttributes with EFI_MEMORY_XP X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 20 Mar 2017 19:31:20 -0000 Content-Type: text/plain; charset=UTF-8 On 20 March 2017 at 15:24, Laszlo Ersek wrote: > On 03/20/17 15:08, Ard Biesheuvel wrote: >> On 20 March 2017 at 11:38, Laszlo Ersek wrote: >>> On 03/20/17 12:20, Ard Biesheuvel wrote: >>>> On 20 March 2017 at 11:16, Michael Zimmermann wrote: >>>>> Ard, why is SetMSetMemorySpaceAttributes being called in first place? >>>>> (ignoring the recent NX patch) >>>>> Looking at the initial GCD, it looks like unused memory usually >>>>> doesn't have any attributes set anyway. >>>>> >>>> >>>> Originally, we added the new memory with >>>> EFI_MEMORY_WB|EFI_MEMORY_WT|EFI_MEMORY_WC|EFI_MEMORY_UC capabilities, >>>> and selected the EFI_MEMORY_WB attribute with the call to >>>> gDS->SetMemorySpaceAttributes. Later, we removed all capabilities >>>> expect EFI_MEMORY_WB, since the other ones cannot be supported under >>>> virtualization with KVM. >>>> >>>> Whether that makes the SetMemorySpaceAttributes redundant is not >>>> entirely clear to me, but it does appear the adding the memory does >>>> the right thing wrt non-exec permissions if the policy is enabled. So >>>> perhaps we can simply drop this call? >>> >>> Won't that turn off caching for the memory just added? >>> >> >> I think it may not map the memory at all in this case, so we need to >> do something. It looks like calling mCpu->SetMemoryAttributes() should >> be sufficient here, and so I wonder whether we violate anything by >> replacing gDS->SetMemorySpaceAttributes with mCpu->SetMemoryAttributes >> here. > > Earlier you mentioned that we cannot get some piece of information from > the GCD map, which limits what we can do here. > > Looking at GetMemorySpaceDescriptor() and GetMemorySpaceMap(), they > should return both Capabilities and Attributes. > > Also, looking at vol2 in PI 1.5, I find: > > * GetMemorySpaceDescriptor() returns EFI_NOT_AVAILABLE_YET if "The > attributes cannot be set because CPU architectural protocol is not > available yet." > > * 9.7.3.2 SetMemorySpaceAttributes() -- When the DXE Foundation is > notified that the EFI_CPU_ARCH_PROTOCOL has been installed, then the DXE > Service SetMemorySpaceAttributes() can be made available. The DXE > Foundation can then use the SetMemoryAttributes() service of the > EFI_CPU_ARCH_PROTOCOL to implement the DXE Service > SetMemorySpaceAttributes(). > > * 9.7.3.3 GetMemorySpaceMap() -- When the DXE Foundation is notified > that the EFI_CPU_ARCH_PROTOCOL has been installed, then the DXE Service > GetMemorySpaceMap() is fully functional. This function is made available > when the memory-based services are initialized. However, the Attributes > field of the array of EFI_GCD_MEMORY_SPACE_DESCRIPTORs is not valid > until the EFI_CPU_ARCH_PROTOCOL is installed. > OK, that's the theory. The reality (as Jiewen confirms) is that the RO/XP capability and attribute bits are never used on the GCD side, because they may cause compatibility issues. The problem is that UEFI memory map entries inherit all attributes from the GCD memory space entries they are carved out of. This means that all UEFI memory map entries will have RO and XP set, which is clearly not what we want. I guess it would be feasible to filter out RO and XP when calling CoreAddRange(), but that does not happen currently. > So, assuming that you have tested GetMemorySpaceMap() earlier, and have > found Attributes useless for the protection (or other) purposes, may > that have happened because the CPU arch protocol was not available yet? > No > ... I guess my email is a bit confusing. My points are, (a) we should > likely not mess directly with the CPU arch protocol if we know (and we > do know) that the GCD services use them internally, (b) are we > absolutely sure that the GCD services can't help us here? > > If nothing else works, I agree we can mess with the CPU arch protocol > directly. > I'm afraid that is our only option.