From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail05.groups.io (mail05.groups.io [45.79.224.7]) by spool.mail.gandi.net (Postfix) with ESMTPS id 54F53AC1B70 for ; Tue, 28 Jan 2025 22:06:27 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=UvpypY+/SM35qpmIuQhJPtIeea+3yjXZ0h9Cex/7jFU=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:User-Agent:Subject:From:To:Cc:References:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Resent-Date:Resent-From:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20240830; t=1738101987; v=1; x=1738361185; b=tRCS7ryhFqMAYd92lwb1AXXDopG+HHVCfzL1PF7GWjgrz2+qlb2S0c55i9UejeOwWH12TkcR VsX4dU4aSp20wA3Sa/VJBZvzK2dUZJZyiQNxevIe8bDEBqZQ84QHkmDDI6qXkPaw0bWzfFC77bh j5/QtRiHVxVgXxhljBwfRSNWOko8DcKRwKT6nDrF2nZOW4PgFDbA0q+L8R/MVoeJMmuyzjdHtLd xyYkoR3nyB3VDtbiWIW93AE6LEcRQYdvau6PS9t4KyhI9abXhQkuWD4DQxT5XYQLBnzk9wUsbLA RFQHgeeKXsvXlWZGqD9h4p06gJt3h2eEchwQU4SScYFlg== X-Received: by 127.0.0.2 with SMTP id 927SYY7687511xev1pSqlF7K; Tue, 28 Jan 2025 14:06:25 -0800 X-Received: from mail-ej1-f54.google.com (mail-ej1-f54.google.com [209.85.218.54]) by mx.groups.io with SMTP id smtpd.web10.34104.1737368841211046223 for ; Mon, 20 Jan 2025 02:27:21 -0800 X-Received: by mail-ej1-f54.google.com with SMTP id a640c23a62f3a-ab322ecd75dso761405866b.0 for ; Mon, 20 Jan 2025 02:27:20 -0800 (PST) X-Forwarded-Encrypted: i=1; AJvYcCUyVjZ1vSrVUUxNsXgNwvwLBKctXnuG4Noun99f5jHRQdM/v0oeSrOSKzwORaiAF9G1ZQ2fzg==@edk2.groups.io X-Gm-Message-State: PfHfj1Q7riVkRj4Qc2f0zvuxx7686176AA= X-Gm-Gg: ASbGncuj2JaCHWDgsDoNEdIjYDvpxGAb1Ge9LSZERz6DZQaVP5o4S1xz7cPH2MmJKpA c2/RCXuI3wPmnCwJUpolwMajHqLiJ+fZMv3j9BRvVV7HCeMtyFuZiXMfAWbXKYdZmW0Q2rdo34c TP+uxrpmgKjatRJJTxv4qz404DZqtrztZqrXL70F+ioFXqoVMKAxqBstO+gtQF5L/apbpbUCoOw wV5sHICW7wg+KYEqQqZRXTPLjAqKi7tK0Fre008L8l25St5guIe9cPFg4XRazca0ykpex7ajoJb VeKNa5TmgCRuFZpQxYVNBG25EYQiZLhgryOVd9DOVQ== X-Google-Smtp-Source: AGHT+IFJVlthfW01rep+roRrPx41HHx1BoP/crq5DSFMeeGWPv+XAPS4A/l3WgF9Nu1tdQ84n+Nkfg== X-Received: by 2002:a17:907:9485:b0:ab3:3b92:8ca5 with SMTP id a640c23a62f3a-ab36e2422camr1516607466b.12.1737368839235; Mon, 20 Jan 2025 02:27:19 -0800 (PST) X-Received: from ?IPV6:2a03:83e0:1126:4:829:739b:3caa:6500? ([2620:10d:c092:500::4:4372]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-ab384ce6890sm596532066b.70.2025.01.20.02.27.18 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 20 Jan 2025 02:27:18 -0800 (PST) Message-ID: Date: Mon, 20 Jan 2025 10:27:18 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [edk2-devel] [RFC 1/2] efi/memattr: Use desc_size instead of total size to check for corruption From: "Usama Arif via groups.io" To: Ard Biesheuvel Cc: linux-efi@vger.kernel.org, devel@edk2.groups.io, kexec@lists.infradead.org, hannes@cmpxchg.org, x86@kernel.org, linux-kernel@vger.kernel.org, leitao@debian.org, gourry@gourry.net, kernel-team@meta.com, Dave Young References: <20250108215957.3437660-1-usamaarif642@gmail.com> <20250108215957.3437660-2-usamaarif642@gmail.com> <8613563a-ee7c-4271-b1f0-4d1ac365ad3a@gmail.com> <138f28ec-341e-4c48-a14b-4371a8198de8@gmail.com> In-Reply-To: <138f28ec-341e-4c48-a14b-4371a8198de8@gmail.com> Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Resent-Date: Tue, 28 Jan 2025 14:06:25 -0800 Resent-From: usamaarif642@gmail.com Reply-To: devel@edk2.groups.io,usamaarif642@gmail.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20240830 header.b=tRCS7ryh; dmarc=pass (policy=none) header.from=groups.io; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 45.79.224.7 as permitted sender) smtp.mailfrom=bounce@groups.io On 13/01/2025 12:00, Usama Arif wrote: >=20 >=20 > On 13/01/2025 11:27, Usama Arif wrote: >> >> >> On 13/01/2025 02:33, Dave Young wrote: >>> On Fri, 10 Jan 2025 at 18:54, Usama Arif wrote= : >>>> >>>> >>>> >>>> On 10/01/2025 07:21, Ard Biesheuvel wrote: >>>>> On Thu, 9 Jan 2025 at 17:36, Usama Arif wrot= e: >>>>>> >>>>>> >>>>>> >>>>>> On 09/01/2025 15:45, Ard Biesheuvel wrote: >>>>>>> On Wed, 8 Jan 2025 at 23:00, Usama Arif wr= ote: >>>>>>>> >>>>>>>> The commit in [1] introduced a check to see if EFI memory attribut= es >>>>>>>> table was corrupted. It assumed that efi.memmap.nr_map remains >>>>>>>> constant, but it changes during late boot. >>>>>>>> Hence, the check is valid during cold boot, but not in the subsequ= ent >>>>>>>> kexec boot. >>>>>>>> >>>>>>>> This is best explained with an exampled. At cold boot, for a test >>>>>>>> machine: >>>>>>>> efi.memmap.nr_map=3D91, >>>>>>>> memory_attributes_table->num_entries=3D48, >>>>>>>> desc_size =3D 48 >>>>>>>> Hence, the check introduced in [1] where 3x the size of the >>>>>>>> entire EFI memory map is a reasonable upper bound for the size of = this >>>>>>>> table is valid. >>>>>>>> >>>>>>>> In late boot __efi_enter_virtual_mode calls 2 functions that updat= es >>>>>>>> efi.memmap.nr_map: >>>>>>>> - efi_map_regions which reduces the `count` of map entries >>>>>>>> (for e.g. if should_map_region returns false) and which is refle= cted >>>>>>>> in efi.memmap by __efi_memmap_init. >>>>>>>> At this point efi.memmap.nr_map becomes 46 in the test machine. >>>>>>>> - efi_free_boot_services which also reduces the number of memory r= egions >>>>>>>> available (for e.g. if md->type or md->attribute is not the righ= t value). >>>>>>>> At this point efi.memmap.nr_map becomes 9 in the test machine. >>>>>>>> Hence when you kexec into a new kernel and pass efi.memmap, the >>>>>>>> paramaters that are compared are: >>>>>>>> efi.memmap.nr_map=3D9, >>>>>>>> memory_attributes_table->num_entries=3D48, >>>>>>>> desc_size =3D 48 >>>>>>>> where the check in [1] is no longer valid with such a low efi.memm= ap.nr_map >>>>>>>> as it was reduced due to efi_map_regions and efi_free_boot_service= s. >>>>>>>> >>>>>>>> A more appropriate check is to see if the description size reporte= d by >>>>>>>> efi and memory attributes table is the same. >>>>>>>> >>>>>>>> [1] https://lore.kernel.org/all/20241031175822.2952471-2-ardb+git@= google.com/ >>>>>>>> >>>>>>>> Fixes: 8fbe4c49c0cc ("efi/memattr: Ignore table if the size is cle= arly bogus") >>>>>>>> Reported-by: Breno Leitao >>>>>>>> Signed-off-by: Usama Arif >>>>>>>> --- >>>>>>>> drivers/firmware/efi/memattr.c | 16 ++++++---------- >>>>>>>> 1 file changed, 6 insertions(+), 10 deletions(-) >>>>>>>> >>>>>>> >>>>>>> The more I think about this, the more I feel that kexec on x86 shou= ld >>>>>>> simply discard this table, and run with the firmware code RWX (whic= h >>>>>>> is not the end of the world). >>>>>> >>>>>> >>>>>> By discard this table, do you mean kexec not use e820_table_firmware= ? >>>>> >>>>> No, I mean kexec ignores the memory attributes table. >>>>> >>>>>> Also a very basic question, what do you mean by run with the firmwar= e RWX? >>>>>> >>>>> >>>>> The memory attributes table is an overlay for the EFI memory map that >>>>> describes which runtime code regions may be mapped with restricted >>>>> permissions. Without this table, everything will be mapped writable a= s >>>>> well as executable, but only in the EFI page tables, which are only >>>>> active when an EFI call is in progress. >>>>> >>>> >>>> Thanks for explaining! >>>> >>>> So basically get rid of memattr.c :) >>>> >>>> Do you mean get rid of it only for kexec, or not do it for any >>>> boot (including cold boot)? >>>> I do like this idea! I couldn't find this in the git history, >>>> but do you know if this was added in the linux kernel just >>>> because EFI spec added support for it, or if there was a >>>> specific security problem? >>>> >>> >>> Usama, can you try the patch below? >>> [ format is wrong due to webmail corruption. But if it works I can >>> send a formal patch later ] >>> >>> $ git diff arch/x86 >>> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/qui= rks.c >>> index 846bf49f2508..58dc77c5210e 100644 >>> --- a/arch/x86/platform/efi/quirks.c >>> +++ b/arch/x86/platform/efi/quirks.c >>> @@ -561,6 +561,11 @@ int __init efi_reuse_config(u64 tables, int nr_tab= les) >>> >>> if (!efi_guidcmp(guid, SMBIOS_TABLE_GUID)) >>> ((efi_config_table_64_t *)p)->table =3D data->s= mbios; >>> + >>> + /* Not bother to play with mem attr table across kexec = */ >>> + if (!efi_guidcmp(guid, EFI_MEMORY_ATTRIBUTES_TABLE_GUID= )) >>> + ((efi_config_table_64_t *)p)->table =3D >>> EFI_INVALID_TABLE_ADDR; >>> + >>> p +=3D sz; >>> } >>> >> >> This would work, I am guessing it will have a similar effect to what I s= ent >> last week in=20 >> https://lore.kernel.org/all/fd63613c-fd26-42de-b5ed-cc734f72eb36@gmail.c= om/ >> >> I think it needs to be wrapped in ifdef CONFIG_X86_64. >> >=20 > IMO we should consider the 2 patches in this series first before disablin= g it for > kexec. These patches actually fix the issue. >=20 Hi Ard, Just wanted to check how should we proceed forward? Should we try and fix t= he warning and corruption during kexec as done in this series or not initialize memory= attributes table at all in kexec boot? I would prefer fixing the issues as in this ser= ies. Thanks, Usama -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#121053): https://edk2.groups.io/g/devel/message/121053 Mute This Topic: https://groups.io/mt/110517813/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-