From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web10.6199.1675880716536070692 for ; Wed, 08 Feb 2023 10:25:16 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=eol11THi; spf=pass (domain: kernel.org, ip: 139.178.84.217, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id B227B6177B for ; Wed, 8 Feb 2023 18:25:15 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 94173C433A7 for ; Wed, 8 Feb 2023 18:25:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1675880714; bh=bXvH8589ZMDVmJ9a3wfXxgc/1f2BTp9xA+M4drGXt7s=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=eol11THidYeNU94JVOqABKhcjdSZvyk9RLs8fQnFLOd+JHnBi6bZ4rZ3thWeFmVqj sP5s6C+IklsyVeZV1CjKRWeE44veBjC0W2BbUhiirH3c2rbhwcPej/EFnWIQpBNs/1 8atynUZ/94ZwtKQuV8qjdl9O4WEUWv+/D7GLps+/fDviWcJYbUZIuDsQEt5iOGIvvZ sFWydMxVGyfiVqNRuIFuWuA7RJ79iL9bV6rNZx5DeXoiy8Ir1BDuurKi7A5ellMBZO jQT7O59uyspT8xLBCkK4bT5jBqJI8+nlordDJ4sAt6hDGBWdJeHdryyoOXKRJzrX3t BPMuDR4r0gG0A== Received: by mail-lj1-f180.google.com with SMTP id d8so20366325ljq.9 for ; Wed, 08 Feb 2023 10:25:14 -0800 (PST) X-Gm-Message-State: AO0yUKW0XDgDRJ6hhTXDRGysODTTpBSFRGQM/fkfeSohgnf3/niJnsEb N9AiBt+hwf516T/LfD9FX0XU3QQM3BlOahE7JVM= X-Google-Smtp-Source: AK7set/OR4obPbBlxWD2tvbwl5UTtm2ZDsOVxbrNy5YPj2AZHicuNL1kME0pefgytHsgcGUZxADZiICA0/4T7T15XT8= X-Received: by 2002:a2e:8206:0:b0:290:5b9d:e97 with SMTP id w6-20020a2e8206000000b002905b9d0e97mr1328106ljg.187.1675880712533; Wed, 08 Feb 2023 10:25:12 -0800 (PST) MIME-Version: 1.0 References: <20230208175812.700129-1-ardb@kernel.org> <20230208175812.700129-4-ardb@kernel.org> In-Reply-To: <20230208175812.700129-4-ardb@kernel.org> From: "Ard Biesheuvel" Date: Wed, 8 Feb 2023 19:25:00 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 3/3] MdeModulePkg/DxeCore: Unconditionally set memory protections To: devel@edk2.groups.io Cc: Michael Kinney , Liming Gao , Jiewen Yao , Michael Kubacki , Sean Brogan , Rebecca Cran , Leif Lindholm , Sami Mujawar , Taylor Beebe , =?UTF-8?Q?Marvin_H=C3=A4user?= Content-Type: text/plain; charset="UTF-8" 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 >