From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x232.google.com (mail-it0-x232.google.com [IPv6:2607:f8b0:4001:c0b::232]) (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 1D11881F52 for ; Thu, 9 Feb 2017 06:55:34 -0800 (PST) Received: by mail-it0-x232.google.com with SMTP id c7so126710161itd.1 for ; Thu, 09 Feb 2017 06:55:34 -0800 (PST) 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=76gSHBjsDrKLgoUsvfaISwbmmIrukQu8TTalsC8xUUM=; b=ecgMu4jDaBY9oyXWhG2gn38sG85pcFy7F+WZYAkxSr/w9vbMD8Nad9lblTj7FQ6eA7 lArI94KfnJwW179C8jDuRlB1zmcjKgoqrdQ5OOzgPBh+qmhxeb/IxUb9yzZ0V4Gos8Uf bU4hoAVzhNL5vjbdkY+UDAkGATaw8KQS5OtL0= 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=76gSHBjsDrKLgoUsvfaISwbmmIrukQu8TTalsC8xUUM=; b=gHfq+QDYNqxf5VgCRGKA1TzUJXoCvCFx9kpaqFTOJC1rkTmJvro7nP5kGDPcwXs1yA 3Ys00ivJOd+4zVzCXIovvcnhUOtjgtuYyVlWB8apQO2xEePrey9wxX1W8mbCnqPhBzKn SbBRJxP1IBAb1IwpNp7AoZwwJgfW7XTdozPh1+SOV2H70WH0I5mMZ0b7MP+D+bytZ7Pe P+x6jNta8Gn0WmcVBvUFwPdhreo3IuAl87UJcGdXYsbxZrSAbnRaDKY1qbIfan9GVtMJ v4PmOp8P6z5edhMo6UQY+ltSZDGbcpRgrXWkbAPAD8c82yasp0hEYwuYVaMTO9V9LEIB 4mmg== X-Gm-Message-State: AMke39lw0mOQ2kT0tJmMY5akwotHxdgCOGCOsSqburnh1HbZpRLtUdGkgPeAFbYpb9PUb1jNswyETjZGGvhlk86K X-Received: by 10.36.74.67 with SMTP id k64mr3455208itb.37.1486652133399; Thu, 09 Feb 2017 06:55:33 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.144.139 with HTTP; Thu, 9 Feb 2017 06:55:32 -0800 (PST) In-Reply-To: <74D8A39837DF1E4DA445A8C0B3885C503A8EBF20@shsmsx102.ccr.corp.intel.com> References: <1486624832-15736-1-git-send-email-jiewen.yao@intel.com> <74D8A39837DF1E4DA445A8C0B3885C503A8EBD52@shsmsx102.ccr.corp.intel.com> <74D8A39837DF1E4DA445A8C0B3885C503A8EBEC3@shsmsx102.ccr.corp.intel.com> <74D8A39837DF1E4DA445A8C0B3885C503A8EBF20@shsmsx102.ccr.corp.intel.com> From: Ard Biesheuvel Date: Thu, 9 Feb 2017 14:55:32 +0000 Message-ID: To: "Yao, Jiewen" Cc: "Tian, Feng" , "edk2-devel@lists.01.org" , Leif Lindholm , "Kinney, Michael D" , "Fan, Jeff" , "Zeng, Star" Subject: Re: [PATCH V3 0/4] DXE Memory Protection X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 09 Feb 2017 14:55:34 -0000 Content-Type: text/plain; charset=UTF-8 On 9 February 2017 at 14:08, Yao, Jiewen wrote: > For X86 CPU, the memory protection attribute goes to page table, the cache > attribute goes to MTRR register. > > Those are 2 difference resource, and can be set separately. > > > > The high level pseudo code is below: > > ======================= > > CacheAttribute = Attribute & CACHE_ATTRIBUTE_MASK > > MemoryProtectionAttribute = Attribute & MEMORY_PROTECTION_ATTRIBUTE_MASK > > If (CacheAttribute != 0) { > > SetCacheAttribute(CacheAttribute) > > } > > SetMemoryProtectionAttribute(MemoryProtectionAttribute) > > ======================= > > > > NOTE: we need compare CacheAttribute == 0, because the cache attribute is an > individual mask. 0x1 means UN_CACHE, 0x8 means WRITE_BACK. 0 is meaningless. > > We do not compare MemoryProtectionAttribute == 0, because 0 is a valid > memory protection attribute, which means to disable protection. > > > > Before GCD sync, the DxeCore does not know the cache attribute, so that it > can only set memory attribute. The CPU driver only modifies page table based > upon MemoryProtectionAttribute and keep cache attribute untouched. > OK, I think we should be able to work around this, although it is not pretty. I will send it out as a separate patch. I do have one other question though: would it be possible to round up the end of the image to page size in this code? (in SetUefiImageProtectionAttributes()) Otherwise, we may end up calling SetMemoryAttributes() with a length that is not page aligned, which hits an EFI_UNSUPPORTED on ARM. Given that the PE/COFF loader always allocates full pages, we know the space after the image is not used, so it is possible (and even more secure!) to clear the exec bit on it. """ // // Last DATA // ASSERT (CurrentBase <= ImageEnd); if (CurrentBase < ImageEnd) { // // DATA // if (Protect) { Attribute = EFI_MEMORY_XP; } else { Attribute = 0; } SetUefiImageMemoryAttributes ( CurrentBase, ImageEnd - CurrentBase, Attribute ); } """