From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by mx.groups.io with SMTP id smtpd.web10.12782.1678975220248992353 for ; Thu, 16 Mar 2023 07:00:20 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=fcvboN+D; spf=pass (domain: kernel.org, ip: 145.40.68.75, 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 ams.source.kernel.org (Postfix) with ESMTPS id 1580CB821A5 for ; Thu, 16 Mar 2023 14:00:18 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id CFFD5C433A0 for ; Thu, 16 Mar 2023 14:00:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1678975216; bh=vQb0/T8MjgzOnMOP5TIN66L9VCsc2uegMmNANnmOwhw=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=fcvboN+DTAdAgXPtwbCV5vVYa9qMlvNh5GTpn1ioK41dKofLXzjOsRkE1j0qMf6Ae oYohebwS7cZfTRNOczv39CC9WRlTbzg9NUw4oHtfhfuTXTJhJGt1S4ed9yuXQjeutE 3cR4XqyXvGJ/o0z5YLduO1j0w53o4lnOXzhDkxi8ujyIRzxkyKK4cyaNounc1KDrGA ff+dqcKcHO3ObgKRDdDh1XHJTSwkALT1G9RHqxAPUQyoZAJRGUb2P+/hJbfXGF3uMD VFer8vnHF1CpFRPKKQOF6DCNFftATJJiOXli+s1OxTf7VNuCQwCsJZl/LKms7lbJhl M8aIL3uDOjI/g== Received: by mail-lj1-f170.google.com with SMTP id y14so1795486ljq.4 for ; Thu, 16 Mar 2023 07:00:16 -0700 (PDT) X-Gm-Message-State: AO0yUKUNdnTXGFCemW8ooTulSW28kRNAtll/TDHt3gIySH7ro40Wx4FH 6OGhsEJm7BKCOD892LnTYFFpFqjjX8KPu2uel84= X-Google-Smtp-Source: AK7set89vKpER08bSb/EL1WJiqA/MCqGkaOaKBUAbNU6w6XX1CGofIZnXnytnxBp9DMrb9Z10mS6wCRDzE2Au2UKQl0= X-Received: by 2002:a05:651c:108:b0:299:ac5e:376e with SMTP id a8-20020a05651c010800b00299ac5e376emr416118ljb.2.1678975214880; Thu, 16 Mar 2023 07:00:14 -0700 (PDT) MIME-Version: 1.0 References: <20230313171714.3866151-1-ardb@kernel.org> <20230313171714.3866151-33-ardb@kernel.org> In-Reply-To: From: "Ard Biesheuvel" Date: Thu, 16 Mar 2023 15:00:03 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH v5 32/38] MdeModulePkg/DxeCore: Update memory protections before freeing a region To: devel@edk2.groups.io, quic_llindhol@quicinc.com Cc: Michael Kinney , Liming Gao , Jiewen Yao , Michael Kubacki , Sean Brogan , Rebecca Cran , Sami Mujawar , Taylor Beebe Content-Type: text/plain; charset="UTF-8" On Thu, 16 Mar 2023 at 14:51, Leif Lindholm wrote: > > On Mon, Mar 13, 2023 at 18:17:08 +0100, Ard Biesheuvel wrote: > > Currently, we invoke ApplyMemoryProtectionPolicy() after > > CoreInternalFreePages() has returned successfully, in order to update > > the memory permission attributes of the region to match the policy for > > EfiConventionalMemory. > > > > There are two problems with that: > > - CoreInternalFreePages() will round up the size of the allocation to > > the appropriate alignment of the memory type, but we only remap the > > number of pages that was passed by the caller, leaving the remaining > > pages freed but mapped with the old permissions; > > - in DEBUG builds, we may attempt to clear the entire region while it is > > still mapped with read-only or read-protect attributes. > > > > Let's address both issues, by updating the permissions before performing > > the actual conversion. > > > > Signed-off-by: Ard Biesheuvel > > --- > > MdeModulePkg/Core/Dxe/Mem/Page.c | 15 ++++++++------- > > 1 file changed, 8 insertions(+), 7 deletions(-) > > > > diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c > > index 5903ce7ab525..f5b940bbc25b 100644 > > --- a/MdeModulePkg/Core/Dxe/Mem/Page.c > > +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c > > @@ -1519,8 +1519,8 @@ CoreAllocatePages ( > > @return EFI_SUCCESS -Pages successfully freed. > > > > **/ > > +STATIC > > EFI_STATUS > > -EFIAPI > > CoreInternalFreePages ( > > This is addressing a historic oversight (possibly caused by the STATIC > function ban), but it's not *really* related to the change in question. > Not entirely. I am moving some logic from a caller into the callee. This is only safe if there are no other callers, and making it STATIC makes it more likely that other callers that may exist in one form or another (i.e., out of tree forks) will fail at build time rather than misbehave. Or that was the rationale, anyway. I'm happy to drop it as well.