From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pj1-f52.google.com (mail-pj1-f52.google.com [209.85.216.52]) by mx.groups.io with SMTP id smtpd.web10.7555.1675883523107558102 for ; Wed, 08 Feb 2023 11:12:03 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@taylorbeebe.com header.s=google header.b=WMAOPVmk; spf=pass (domain: taylorbeebe.com, ip: 209.85.216.52, mailfrom: t@taylorbeebe.com) Received: by mail-pj1-f52.google.com with SMTP id mi9so19333583pjb.4 for ; Wed, 08 Feb 2023 11:12:03 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=taylorbeebe.com; s=google; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=aCUJlpETKN3Adp9RDCCVyJheTjkVfGkG2+/kHcBO6pM=; b=WMAOPVmkamlj3DBs8R6PvighL6D0QEWF3Asnf57Klg4CXXJSgnsQjYsOvsQ3SRcijk UgmuwOypOexqTxjQ2AzvNJZ367yHnsnh9LeVC6ar+mun7XQzQmAeDvgd7ydLjPiMjll0 2yqHYAWyy8L+SDNWVH7WPiETIVWVXXfHg70wPFJ4qKQFLFLWNpMsOxTlYQnH/fFsQQAV pG+8uJj68w0pioOgKtwjS/7ssf/tJlbPOO9Cus0cYH+1p3uPmKqMUO8NxkUjcRwq0YxV vHPHzqYIuVXHlvjxVaDOqRWE1AAUAYilHNTUgXe9hgfHABbrqQQEYJ2geg8z2pRGQpv3 fh8Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=aCUJlpETKN3Adp9RDCCVyJheTjkVfGkG2+/kHcBO6pM=; b=DqmPDHL6HLtTUAunUzJwT/GdRe3bZQjomcp+sl26feGTwls1NAEqKfMU1L/xsmy3Cb yQnK469gAmVCw8LdbCgcZujuCu2ZB8tlCY41RK9n1p3abg3bEiPRV0YikVfkzSKoeoen eEvbqGyrsf4KhZUnM/Fiw8FkE1bI7SR0ouCSEbCv7PG2LVA61y8jMuD9RSenm7cXGySt Q6itNBKoGA43hCzRmqC3QgNP2he6HgBngqWxMRiERo8MuYcG8CrO9hQGeIMLZmYm7Fp5 rJh3wNmXkShHQ6ixSJs1IZB4qYIl9cHW6OfzncFac1hiMOYMa2cVv60BiPoFMjMO/y0k R9Tg== X-Gm-Message-State: AO0yUKWxKh8a1PIKp3EmX3fUyKofDtXPhx4eCF9M4MC9/dNicm5SsTvt r0eirDr3l7WhZrHfB/YIBP2Mvw== X-Google-Smtp-Source: AK7set9k78tuy9d5PLiT8NSN04K6j1iFOzhKJzXcJbkNHoNZuRjwcbzz72rzUqrQjf4rsUmbKvY31A== X-Received: by 2002:a17:90b:3b8b:b0:231:1c47:1654 with SMTP id pc11-20020a17090b3b8b00b002311c471654mr2559226pjb.27.1675883522333; Wed, 08 Feb 2023 11:12:02 -0800 (PST) Return-Path: Received: from [192.168.50.162] ([50.46.230.135]) by smtp.gmail.com with ESMTPSA id r14-20020a17090a1bce00b002310ed024adsm1911296pjr.12.2023.02.08.11.12.01 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 08 Feb 2023 11:12:02 -0800 (PST) Message-ID: <32ae5b04-0a0c-c8c4-7b2d-cee3ac2a2517@taylorbeebe.com> Date: Wed, 8 Feb 2023 11:12:01 -0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.6.1 Subject: Re: [PATCH 3/3] MdeModulePkg/DxeCore: Unconditionally set memory protections To: Ard Biesheuvel , devel@edk2.groups.io Cc: Michael Kinney , Liming Gao , Jiewen Yao , Michael Kubacki , Sean Brogan , Rebecca Cran , Leif Lindholm , Sami Mujawar , =?UTF-8?Q?Marvin_H=c3=a4user?= References: <20230208175812.700129-1-ardb@kernel.org> <20230208175812.700129-4-ardb@kernel.org> From: "Taylor Beebe" In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit I ran some tests and did some quick napkin math. Based on the time it takes to perform the SetMemoryAttributes() routine on QEMU, as long as <79% of the calls to ApplyMemoryProtectionPolicy() actually require a subsequent call to SetMemoryAttributes(), it is more efficient to walk the page/translation table to check if the attributes actually need to be updated. Even on a platform with varied NX policy settings, the number of times the attributes need to be updated is less than 0.0005% of all calls to ApplyMemoryProtectionPolicy() (likely due to most allocations being BootServicesData). Once the ARM and X64 implementations of the Memory Attribute Protocol are in, would you be open to updating this block to utilize the protocol to check the attributes of the region being updated? On 2/8/2023 10:25 AM, Ard Biesheuvel wrote: > On Wed, 8 Feb 2023 at 18:58, Ard Biesheuvel wrote: >> >> Instead of relying on a questionable heuristic that avoids calling into >> the SetMemoryAttributes () DXE service when the old memory type and the >> new one are subjected to the same NX memory protection policy, make this >> call unconditionally. This avoids corner cases where memory region >> attributes are out of sync with the policy, either due to the fact that >> we are in the middle of ramping up the protections, or due to explicit >> invocations of SetMemoryAttributes() by drivers. >> >> This requires the architecture page table code to be able to deal with >> this, in particular, it needs to be robust against potential recursion >> due to NX policies being applied to newly allocated page tables. >> >> Signed-off-by: Ard Biesheuvel >> --- >> MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 29 -------------------- >> 1 file changed, 29 deletions(-) >> >> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c >> index 36987843f142..503feb72b5d0 100644 >> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c >> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c >> @@ -1263,9 +1263,7 @@ ApplyMemoryProtectionPolicy ( >> IN UINT64 Length >> ) >> { >> - UINT64 OldAttributes; >> UINT64 NewAttributes; >> - EFI_STATUS Status; >> >> // >> // The policy configured in PcdDxeNxMemoryProtectionPolicy >> @@ -1320,32 +1318,5 @@ ApplyMemoryProtectionPolicy ( >> // >> NewAttributes = GetPermissionAttributeForMemoryType (NewType); >> >> - if (OldType != EfiMaxMemoryType) { >> - OldAttributes = GetPermissionAttributeForMemoryType (OldType); >> - if (!mAfterDxeNxMemoryProtectionInit && >> - (OldAttributes == NewAttributes)) { >> - return EFI_SUCCESS; >> - } >> - > > This removes some code that does not actually exist - apologies. > > It comes down to just removing the conditional checks here, though, > and perform the tail call below unconditionally. > >> - // >> - // If available, use the EFI memory attribute protocol to obtain >> - // the current attributes of the region. If the entire region is >> - // covered and the attributes match, we don't have to do anything. >> - // >> - if (mMemoryAttribute != NULL) { >> - Status = mMemoryAttribute->GetMemoryAttributes (mMemoryAttribute, >> - Memory, >> - Length, >> - &OldAttributes >> - ); >> - if (!EFI_ERROR (Status) && (OldAttributes == NewAttributes)) { >> - return EFI_SUCCESS; >> - } >> - } >> - } else if (NewAttributes == 0) { >> - // newly added region of a type that does not require protection >> - return EFI_SUCCESS; >> - } >> - >> return gCpu->SetMemoryAttributes (gCpu, Memory, Length, NewAttributes); >> } >> -- >> 2.39.1 >>