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 80EDED80FF3 for ; Thu, 9 Jan 2025 15:45:35 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=FfSldcDkXNrKbItCqfOTLK0gO4lHJcNUiTe/Mk0FoJ0=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Resent-Date:Resent-From:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20240830; t=1736437535; v=1; x=1736696734; b=ArWKjssJF6hN9KSm61WYzBTW16348EYCD2Xala707wR7zEm9x7PxZD51XUR8oQUBC5y3x+FV SXi7JYizsEi1lvKuFXe0SkgmoxEesN1TMGSeW46ZwHhYV62PGW2qSzl7Dc7j5DztAuFVBgoStfD 5i/x3MjF9uWgzg5gUlkrY1S7fMjjbhJQ8ffMutFgh0gIbnKL/0275meXy8WozNIW3BPF1+adyT4 oWQCWntxydINNT8nxcI4xzEY/npzaNwn/vUQCdt2G00qWLOwV93DF+8UTpO7LLUyTSFxGjdhLc7 41hfjuNIWj5v4tKehz+oudmdjqEvE9qc6TCDawbB0j8Bw== X-Received: by 127.0.0.2 with SMTP id bUHbYY7687511xZFycbWQSPj; Thu, 09 Jan 2025 07:45:34 -0800 X-Received: from nyc.source.kernel.org (nyc.source.kernel.org [147.75.193.91]) by mx.groups.io with SMTP id smtpd.web11.51811.1736437533163968056 for ; Thu, 09 Jan 2025 07:45:33 -0800 X-Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by nyc.source.kernel.org (Postfix) with ESMTP id A769FA41F46 for ; Thu, 9 Jan 2025 15:43:43 +0000 (UTC) X-Received: by smtp.kernel.org (Postfix) with ESMTPSA id D9527C4CEE2 for ; Thu, 9 Jan 2025 15:45:31 +0000 (UTC) X-Received: by mail-lj1-f174.google.com with SMTP id 38308e7fff4ca-3022c6155edso8373761fa.2 for ; Thu, 09 Jan 2025 07:45:31 -0800 (PST) X-Forwarded-Encrypted: i=1; AJvYcCURBlQH3HXTvgndJCV9F4ldUiUihBz+LrhUNoh6uC5jqUuYOBzqrgPYqKk6qBZ0+XV2gjVb9A==@edk2.groups.io X-Gm-Message-State: 0O2lPDgzjcuAoO7rCmJ5WgECx7686176AA= X-Google-Smtp-Source: AGHT+IFGG2VZLnb6Rf9cPbXGfRKdHD2AX/axHtZpfvjPsUUlrem6ul0qd2lRvBmBrREq0G7sO/Ktr8Z6TbVGz2UNujk= X-Received: by 2002:a05:6512:b8b:b0:542:249c:2156 with SMTP id 2adb3069b0e04-5428452b8bcmr2118447e87.15.1736437530239; Thu, 09 Jan 2025 07:45:30 -0800 (PST) MIME-Version: 1.0 References: <20250108215957.3437660-1-usamaarif642@gmail.com> <20250108215957.3437660-2-usamaarif642@gmail.com> In-Reply-To: <20250108215957.3437660-2-usamaarif642@gmail.com> From: "Ard Biesheuvel via groups.io" Date: Thu, 9 Jan 2025 16:45:18 +0100 X-Gmail-Original-Message-ID: X-Gm-Features: AbW1kvY3sSuQK6Y-79x26wc0_PSSMv_8bcWle1lLxS4GweITKQnpqDTYjg3HPTc Message-ID: Subject: Re: [edk2-devel] [RFC 1/2] efi/memattr: Use desc_size instead of total size to check for corruption To: Usama Arif Cc: linux-efi@vger.kernel.org, devel@edk2.groups.io, kexec@lists.infradead.org, hannes@cmpxchg.org, dyoung@redhat.com, x86@kernel.org, linux-kernel@vger.kernel.org, leitao@debian.org, gourry@gourry.net, kernel-team@meta.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: Thu, 09 Jan 2025 07:45:33 -0800 Resent-From: ardb@kernel.org Reply-To: devel@edk2.groups.io,ardb@kernel.org List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: text/plain; charset="UTF-8" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20240830 header.b=ArWKjssJ; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 45.79.224.7 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=pass (policy=none) header.from=groups.io On Wed, 8 Jan 2025 at 23:00, Usama Arif wrote: > > The commit in [1] introduced a check to see if EFI memory attributes > 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 subsequent > kexec boot. > > This is best explained with an exampled. At cold boot, for a test > machine: > efi.memmap.nr_map=91, > memory_attributes_table->num_entries=48, > desc_size = 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 updates > 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 reflected > 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 regions > available (for e.g. if md->type or md->attribute is not the right 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=9, > memory_attributes_table->num_entries=48, > desc_size = 48 > where the check in [1] is no longer valid with such a low efi.memmap.nr_map > as it was reduced due to efi_map_regions and efi_free_boot_services. > > A more appropriate check is to see if the description size reported 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 clearly 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 should simply discard this table, and run with the firmware code RWX (which is not the end of the world). The main reason is that the EFI memory map and the EFI memory attributes table are supposed to be a matched pair, where each RTcode entry in the former is broken down into multiple code and data segments in the latter. The amount of mangling that the x86 arch code does of the EFI memory map makes it intractable to ensure that they remain in sync, and so it is better not to bother. > diff --git a/drivers/firmware/efi/memattr.c b/drivers/firmware/efi/memattr.c > index c38b1a335590..d3bc161361fb 100644 > --- a/drivers/firmware/efi/memattr.c > +++ b/drivers/firmware/efi/memattr.c > @@ -40,21 +40,17 @@ int __init efi_memattr_init(void) > goto unmap; > } > > - > /* > - * Sanity check: the Memory Attributes Table contains up to 3 entries > - * for each entry of type EfiRuntimeServicesCode in the EFI memory map. > - * So if the size of the table exceeds 3x the size of the entire EFI > - * memory map, there is clearly something wrong, and the table should > - * just be ignored altogether. > + * Sanity check: the Memory Attributes Table desc_size and > + * efi.memmap.desc_size should match. > */ > - size = tbl->num_entries * tbl->desc_size; > - if (size > 3 * efi.memmap.nr_map * efi.memmap.desc_size) { > - pr_warn(FW_BUG "Corrupted EFI Memory Attributes Table detected! (version == %u, desc_size == %u, num_entries == %u)\n", > - tbl->version, tbl->desc_size, tbl->num_entries); > + if (efi.memmap.desc_size != tbl->desc_size) { > + pr_warn(FW_BUG "Corrupted EFI Memory Attributes Table detected! (version == %u, table desc_size == %u, efi.memmap.desc_size == %lu, table num_entries == %u)\n", > + tbl->version, tbl->desc_size, efi.memmap.desc_size, tbl->num_entries); > goto unmap; > } > > + size = tbl->num_entries * tbl->desc_size; > tbl_size = sizeof(*tbl) + size; > memblock_reserve(efi_mem_attr_table, tbl_size); > set_bit(EFI_MEM_ATTR, &efi.flags); > -- > 2.43.5 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#120975): https://edk2.groups.io/g/devel/message/120975 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] -=-=-=-=-=-=-=-=-=-=-=-