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.90373.1675792617881677366 for ; Tue, 07 Feb 2023 09:56:58 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=WOD5cJlQ; 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 1EE1C60F1F for ; Tue, 7 Feb 2023 17:56:57 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 85BE3C433D2 for ; Tue, 7 Feb 2023 17:56:56 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1675792616; bh=Cyvu4tbhca5lLHUspUa4R3Ha+WiypYw10Z4SEs3BVCQ=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=WOD5cJlQ7fkqcQy41QNXdQiMly2eFUxwdMCy4kFL//jgCncIhr2foJlFmbGUclc9M iDJ0014NxpFrkdFoB6GK5kd6uOTe17jje1zwOQa8FHyfrB2YHtJMaoh4GOlWX3HDey B65SVrI2BGK63FOObNa652R3pYtSXfn/ExXi6zlytNKhdnIOtHaVsELqZKtqZBZBqi 75loAsqfMOWeXv9Kx4BXbIMEHQW7YyfbwmoHa+TJ1WELaMXnWDmXAtQzg2narZ2sz6 XBcvObrkiDF5Vit/0BQ0AJa0CdgVN5L0OCRAqM8dToiaYRLyvwDNol942c2cWxODdq GWawL26dv9RZQ== Received: by mail-lf1-f51.google.com with SMTP id y25so23393373lfa.9 for ; Tue, 07 Feb 2023 09:56:56 -0800 (PST) X-Gm-Message-State: AO0yUKVTNm5oH+OlKUQYu2N7wm4dCNbvn+OhdIV1ndvqI5MQ1EItSXbl DPAB+LiRLd0hpVA9Mkiw57B4tnILKHdQ6KHlJwo= X-Google-Smtp-Source: AK7set/Ov4RurpmlUkAnFLBGiWjeuz9JEhZJIcvOawRMtBNlGRAJaT6RCJ1OXC5hEGTaWE2+SV6dnT+pF2fvGr9rE5E= X-Received: by 2002:ac2:559b:0:b0:4b6:fae9:c9b9 with SMTP id v27-20020ac2559b000000b004b6fae9c9b9mr687719lfg.207.1675792614566; Tue, 07 Feb 2023 09:56:54 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: "Ard Biesheuvel" Date: Tue, 7 Feb 2023 18:56:43 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH 4/4] ArmPkg/CpuDxe: Implement EFI memory attributes protocol To: =?UTF-8?Q?Marvin_H=C3=A4user?= Cc: devel@edk2.groups.io, t@taylorbeebe.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, 7 Feb 2023 at 11:13, Marvin H=C3=A4user wrote: > > > On 7. Feb 2023, at 11:01, Ard Biesheuvel wrote: > > Actually, it seems UnprotectUefiImage () is corrent under the > assumption that all code regions have EFI_MEMORY_XP cleared by > default. > > However, if you redefine the policy to set EFI_MEMORY_XP on code > regions by default, and only permit execution after remapping the code > read-only explicitly, and only then clearing EFI_MEMORY_XP, that > routine should revert the region to EFI_MEMORY_XP. But given the > existing ASSERT()s on having EFI_MEMORY_XP cleared for all code > regions, the code as it is currently is not incorrect. > > > Right. My main issue is, it=E2=80=99s nowhere documented that manually ch= anged permissions must be restored to their default before freeing. Within = DxeCore, this is easily done using the PCDs, but outside (say you allocate = a trampoline buffer and then free it), you would need to manually query the= permissions, store them, and restore later. > Agreed. However, I'd still prefer to only call SetMemorySpaceAttributes() if needed, and setting the same attributes on the entire image allocation at least permits us to double check whether the new attributes are already set on a region, and avoid the call if that is the case. Perhaps we should just set EFI_MEMORY_XP on all images regardless of the policy - the memory should no longer be executable anyway, and what we currently do is remap the entire region RWX after it has executed, which is kind of nasty. > I did *not* look into the implementation code in detail, but does the new= memory permission protocol impose the same constraint implementation-wise = and if so, is this documented anywhere? > Not sure I follow you: which constraint is that? > PS: Fetched the wrong link in my last mail: https://lkml.org/lkml/2022/12= /15/352 > Yeah saw that. I'm hoping to get that in for v6.4 as we missed v6.3 by now. (I did take his patch that adds the definition of the EFI memory attribute protocol only, as I need it for EFI zboot)