From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id A8398D81164 for ; Thu, 7 Dec 2023 16:16:29 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=l/uSIvPbVvp8jy7sobX0XSRQ2V432hOwihPfYkxv/J0=; 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:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1701965788; v=1; b=l4rStONpNVfx26XQTPghd6vWOohBP8JCpMURfGI8+o6S33V/0jaNOg9jmiOq/Tg/ShR7PJ30 Hvj48AokhOPgJ2tc5iJ/ZeHy8u+AxXBKgb4n5ZptCuUWTg1gNv1fq17j8ZbtGa2Mu0ykNtp3Sx7 A8jcby8Pp0HzTUdDQiu5rgCg= X-Received: by 127.0.0.2 with SMTP id HBQWYY7687511xHL4dOc6KDg; Thu, 07 Dec 2023 08:16:28 -0800 X-Received: from sin.source.kernel.org (sin.source.kernel.org [145.40.73.55]) by mx.groups.io with SMTP id smtpd.web10.88510.1701965787097749240 for ; Thu, 07 Dec 2023 08:16:27 -0800 X-Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sin.source.kernel.org (Postfix) with ESMTP id 3DA8FCE1FAD for ; Thu, 7 Dec 2023 16:16:24 +0000 (UTC) X-Received: by smtp.kernel.org (Postfix) with ESMTPSA id 7CFD7C433C8 for ; Thu, 7 Dec 2023 16:16:23 +0000 (UTC) X-Received: by mail-lj1-f175.google.com with SMTP id 38308e7fff4ca-2c9efa1ab7fso11981441fa.0 for ; Thu, 07 Dec 2023 08:16:23 -0800 (PST) X-Gm-Message-State: xJbBqmxUTzYwCCs9aH0gUrdKx7686176AA= X-Google-Smtp-Source: AGHT+IFmKKA9RdA+asi8LnMCp07h9SfCHLlZNHSIwa4yofLzhN+UXySxHLA3DHwpFdUcATD6ICBExGDgNssHTl+boUU= X-Received: by 2002:a2e:8907:0:b0:2c9:fbb9:f1bb with SMTP id d7-20020a2e8907000000b002c9fbb9f1bbmr930106lji.135.1701965781686; Thu, 07 Dec 2023 08:16:21 -0800 (PST) MIME-Version: 1.0 References: <20231207094404.270381-1-kraxel@redhat.com> In-Reply-To: <20231207094404.270381-1-kraxel@redhat.com> From: "Ard Biesheuvel" Date: Thu, 7 Dec 2023 17:16:10 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH v2 1/1] OvmfPkg/VirtNorFlashDxe: sanity-check variables To: Gerd Hoffmann Cc: devel@edk2.groups.io, mike.maslenkin@gmail.com, Jiewen Yao , Ard Biesheuvel , oliver@redhat.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 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=20140610 header.b=l4rStONp; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=kernel.org (policy=none) Hi Gerd, On Thu, 7 Dec 2023 at 10:44, Gerd Hoffmann wrote: > > Extend the ValidateFvHeader function, additionally to the header checks > walk over the list of variables and sanity check them. > > In case we find inconsistencies indicating variable store corruption > return EFI_NOT_FOUND so the variable store will be re-initialized. > > Signed-off-by: Gerd Hoffmann This seems like a good idea to me, and the change looks generally fine, except for one nit below: > --- > OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c | 82 +++++++++++++++++++++-- > 1 file changed, 77 insertions(+), 5 deletions(-) > > diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c > index 5ee98e9b595a..1bfb14495abd 100644 > --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c > +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c > @@ -185,11 +185,16 @@ ValidateFvHeader ( > IN NOR_FLASH_INSTANCE *Instance > ) > { > - UINT16 Checksum; > - EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader; > - VARIABLE_STORE_HEADER *VariableStoreHeader; > - UINTN VariableStoreLength; > - UINTN FvLength; > + UINT16 Checksum; > + EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader; > + VARIABLE_STORE_HEADER *VariableStoreHeader; > + UINTN VarOffset; > + AUTHENTICATED_VARIABLE_HEADER *VarHeader; > + UINTN VarSize; > + CHAR16 *VarName; > + CHAR8 *VarState; > + UINTN VariableStoreLength; > + UINTN FvLength; > > FwVolHeader = (EFI_FIRMWARE_VOLUME_HEADER *)Instance->RegionBaseAddress; > > @@ -260,6 +265,73 @@ ValidateFvHeader ( > return EFI_NOT_FOUND; > } > > + // check variables > + DEBUG ((DEBUG_INFO, "%a: checking variables\n", __func__)); > + VarOffset = sizeof (*VariableStoreHeader); > + while (VarOffset + sizeof (*VarHeader) < VariableStoreHeader->Size) { > + VarHeader = (VOID *)((UINTN)VariableStoreHeader + VarOffset); > + if (VarHeader->StartId != 0x55aa) { > + DEBUG ((DEBUG_INFO, "%a: end of var list\n", __func__)); > + break; > + } > + > + VarSize = sizeof (*VarHeader) + VarHeader->NameSize + VarHeader->DataSize; > + if (VarOffset + VarSize > VariableStoreHeader->Size) { > + DEBUG (( > + DEBUG_ERROR, > + "%a: invalid variable size: 0x%x + 0x%x + 0x%x + 0x%x > 0x%x\n", > + __func__, > + VarOffset, > + sizeof (*VarHeader), > + VarHeader->NameSize, > + VarHeader->DataSize, > + VariableStoreHeader->Size > + )); > + return EFI_NOT_FOUND; > + } > + > + VarName = (VOID *)((UINTN)VariableStoreHeader + VarOffset > + + sizeof (*VarHeader)); > + switch (VarHeader->State) { > + case VAR_HEADER_VALID_ONLY: > + VarState = "header-ok"; > + VarName = L""; > + break; > + case VAR_ADDED: > + VarState = "ok"; > + break; > + case VAR_ADDED &VAR_IN_DELETED_TRANSITION: This looks odd, so please add a comment explaining why these constants are constructed this way. > + VarState = "del-in-transition"; > + break; > + case VAR_ADDED &VAR_DELETED: > + case VAR_ADDED &VAR_DELETED &VAR_IN_DELETED_TRANSITION: > + VarState = "deleted"; > + break; > + default: > + DEBUG (( > + DEBUG_ERROR, > + "%a: invalid variable state: 0x%x\n", > + __func__, > + VarHeader->State > + )); > + return EFI_NOT_FOUND; > + } > + > + DEBUG (( > + DEBUG_VERBOSE, > + "%a: +0x%04x: name=0x%x data=0x%x '%s' (%a)\n", > + __func__, > + VarOffset, > + VarHeader->NameSize, > + VarHeader->DataSize, > + VarName, > + VarState > + )); > + > + VarSize = (VarSize + 3) & ~3; // align > + VarOffset += VarSize; > + } > + > return EFI_SUCCESS; > } > > -- > 2.43.0 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112202): https://edk2.groups.io/g/devel/message/112202 Mute This Topic: https://groups.io/mt/103031342/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-