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 9CC71740047 for ; Mon, 30 Oct 2023 12:39:09 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=c1B8Egba6fLQ2eyl/ibMtdQGaD+LcPR0sFy7G8IR7wk=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1698669548; v=1; b=nnmcNjMeIQPLVQPF8fUyc8zLYXdvtrc5PoGmFVSi5/JamdEfFQqA35wBam3uwRVdDkSh4lBa kwqGxGBCBh7KJBB3mc077S9OE3+O6A1YA/2M27t0aeujKyRlXZJYAycy/P8C6MZMGMdZgpwIvbT dze+a7NZsfdVs5R70Od6ppaM= X-Received: by 127.0.0.2 with SMTP id eXkDYY7687511x7wISr6glKQ; Mon, 30 Oct 2023 05:39:08 -0700 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web10.147627.1698669547629614363 for ; Mon, 30 Oct 2023 05:39:07 -0700 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-470-whkAeHcrMJi3KuF6zaeHmA-1; Mon, 30 Oct 2023 08:38:58 -0400 X-MC-Unique: whkAeHcrMJi3KuF6zaeHmA-1 X-Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id AF093811E88; Mon, 30 Oct 2023 12:38:57 +0000 (UTC) X-Received: from [10.39.194.199] (unknown [10.39.194.199]) by smtp.corp.redhat.com (Postfix) with ESMTPS id B507F2026D66; Mon, 30 Oct 2023 12:38:56 +0000 (UTC) Message-ID: <7a1106c7-ab64-f3fb-dc6b-fdd62ca836b7@redhat.com> Date: Mon, 30 Oct 2023 13:38:55 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v3 3/4] StandaloneMmPkg/Core: Fix issue that section address might be wrong To: Wei6 Xu , devel@edk2.groups.io Cc: Ard Biesheuvel , Sami Mujawar , Ray Ni References: From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.4 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: 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,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: L1iLGVozBuWafa5QEMrVygN1x7686176AA= 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=20140610 header.b=nnmcNjMe; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On 10/30/23 08:49, Wei6 Xu wrote: > MmCoreFfsFindMmDriver() assumes FileHeader is EFI_FFS_FILE_HEADER. > If FileHeader is an EFI_FFS_FILE_HEADER2, 'FileHeader + 1' will get a > wrong section address. Use FfsFindSection to get the section directly, > instead of 'FileHeader + 1' to avoid this issue. >=20 > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Cc: Sami Mujawar > Cc: Ray Ni > Signed-off-by: Wei6 Xu > --- > StandaloneMmPkg/Core/FwVol.c | 18 ++++++++---------- > 1 file changed, 8 insertions(+), 10 deletions(-) >=20 > diff --git a/StandaloneMmPkg/Core/FwVol.c b/StandaloneMmPkg/Core/FwVol.c > index 9d0ce66ef839..fa335d62c252 100644 > --- a/StandaloneMmPkg/Core/FwVol.c > +++ b/StandaloneMmPkg/Core/FwVol.c > @@ -116,23 +116,21 @@ MmCoreFfsFindMmDriver ( > break; > } > =20 > - Status =3D FfsFindSectionData ( > + Status =3D FfsFindSection ( > EFI_SECTION_GUID_DEFINED, > FileHeader, > - &SectionData, > - &SectionDataSize > + &Section > ); > if (EFI_ERROR (Status)) { > break; > } > =20 > - Section =3D (EFI_COMMON_SECTION_HEADER *)(FileHeader + 1); > - Status =3D ExtractGuidedSectionGetInfo ( > - Section, > - &DstBufferSize, > - &ScratchBufferSize, > - &SectionAttribute > - ); > + Status =3D ExtractGuidedSectionGetInfo ( > + Section, > + &DstBufferSize, > + &ScratchBufferSize, > + &SectionAttribute > + ); > if (EFI_ERROR (Status)) { > break; > } (1) Can you remove the SectionData and SectionDataSize variables as well? I think they are unused at this point. With that: Reviewed-by: Laszlo Ersek Ah wait, you're going to use those variables in the next patch, again. OK then. Just take my R-b for this patch. (2) Now that I'm looking at the code in more depth, I don't even understand what the original intent of the FfsFindSectionData() call was! The output values SectionData and SectionDataSize were not used for anything! So it seems like FfsFindSectionData() was called just to make sure an EFI_SECTION_GUID_DEFINED section *existed*. And then we'd treat the very *first* section after the file header -- not too robustly identified, at that -- as a GUIDed section, for extracting its info. So this patch actually fixes two warts: one, the file header size is now considered more generally, two, we don't just assume that the very first section is the GUID-defined section, but look it up. Phew. Laszlo -=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 (#110315): https://edk2.groups.io/g/devel/message/110315 Mute This Topic: https://groups.io/mt/102270548/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-