From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx0a-0031df01.pphosted.com (mx0a-0031df01.pphosted.com [205.220.168.131]) by mx.groups.io with SMTP id smtpd.web10.14164.1678978349558310396 for ; Thu, 16 Mar 2023 07:52:29 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@quicinc.com header.s=qcppdkim1 header.b=OXJ7qPw0; spf=permerror, err=parse error for token &{10 18 %{ir}.%{v}.%{d}.spf.has.pphosted.com}: invalid domain name (domain: quicinc.com, ip: 205.220.168.131, mailfrom: quic_llindhol@quicinc.com) Received: from pps.filterd (m0279865.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 32GEEISb016625; Thu, 16 Mar 2023 14:52:19 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=quicinc.com; h=message-id : date : mime-version : subject : to : cc : references : from : in-reply-to : content-type : content-transfer-encoding; s=qcppdkim1; bh=2rtWzzHbnWrZW9+yBHE3NPMkttAC2uS3wsngjfAOZA0=; b=OXJ7qPw0mcg8cfNxGZU3kUjZnAw6k+ekGvFV558W/dTLzgNeTEWW9qID9h2MgBNF4N7x aO5wDPsXGeb1p4lhYcEU7GP5R8Bcy33r/d9TbItJ/Sc3nsH5IJu8TT3HZ4D74TaldAMA vfnaEygKxI8cYAgNxK4zHDiM8/T4/Q/2aF69g4UDBOyNFtnfi4C1MVnoJ/Te400pzntB dXzCHHhxP+KXCqqGNwqmv1Y2CM+FHmgHyW2vZkGSVAp9TtqgfdBt7LoMyZs6OeLgFVsy /TIzQLU9vckdTcSTT2otI4ePNVF/MFECo2kb9xOQ6qXCASvq+7jZI4ZTQeZEY4cmbaYW nA== Received: from nasanppmta02.qualcomm.com (i-global254.qualcomm.com [199.106.103.254]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 3pbpxsj6gu-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 16 Mar 2023 14:52:19 +0000 Received: from nasanex01c.na.qualcomm.com (nasanex01c.na.qualcomm.com [10.45.79.139]) by NASANPPMTA02.qualcomm.com (8.17.1.5/8.17.1.5) with ESMTPS id 32GEqIEe020240 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 16 Mar 2023 14:52:18 GMT Received: from [10.251.41.119] (10.80.80.8) by nasanex01c.na.qualcomm.com (10.45.79.139) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.986.41; Thu, 16 Mar 2023 07:52:11 -0700 Message-ID: Date: Thu, 16 Mar 2023 14:52:08 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.8.0 Subject: Re: [edk2-devel] [PATCH v5 32/38] MdeModulePkg/DxeCore: Update memory protections before freeing a region To: Ard Biesheuvel , CC: Michael Kinney , Liming Gao , Jiewen Yao , Michael Kubacki , Sean Brogan , Rebecca Cran , Sami Mujawar , Taylor Beebe References: <20230313171714.3866151-1-ardb@kernel.org> <20230313171714.3866151-33-ardb@kernel.org> From: "Leif Lindholm" In-Reply-To: X-Originating-IP: [10.80.80.8] X-ClientProxiedBy: nasanex01a.na.qualcomm.com (10.52.223.231) To nasanex01c.na.qualcomm.com (10.45.79.139) X-QCInternal: smtphost X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=5800 signatures=585085 X-Proofpoint-GUID: gEcH2qyYVQa0xVSAFzfrmmH4UAwfOuTi X-Proofpoint-ORIG-GUID: gEcH2qyYVQa0xVSAFzfrmmH4UAwfOuTi X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.254,Aquarius:18.0.942,Hydra:6.0.573,FMLib:17.11.170.22 definitions=2023-03-16_10,2023-03-16_01,2023-02-09_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 phishscore=0 lowpriorityscore=0 adultscore=0 priorityscore=1501 malwarescore=0 impostorscore=0 mlxscore=0 mlxlogscore=999 suspectscore=0 bulkscore=0 clxscore=1015 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2303150002 definitions=main-2303160120 Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit On 2023-03-16 14:00, Ard Biesheuvel wrote: > 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. I'm OK with that rationale, but I think in that case it needs calling out in the commit message. / Leif