From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.31, mailfrom: michael.d.kinney@intel.com) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by groups.io with SMTP; Tue, 01 Oct 2019 10:31:04 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 01 Oct 2019 10:31:03 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,571,1559545200"; d="scan'208,217";a="391270437" Received: from orsmsx106.amr.corp.intel.com ([10.22.225.133]) by fmsmga005.fm.intel.com with ESMTP; 01 Oct 2019 10:31:01 -0700 Received: from orsmsx158.amr.corp.intel.com (10.22.240.20) by ORSMSX106.amr.corp.intel.com (10.22.225.133) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 1 Oct 2019 10:30:59 -0700 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.232]) by ORSMSX158.amr.corp.intel.com ([169.254.10.46]) with mapi id 14.03.0439.000; Tue, 1 Oct 2019 10:30:59 -0700 From: "Michael D Kinney" To: "devel@edk2.groups.io" , "jason.spottswood@hpe.com" , "Rothman, Michael A" , "Kinney, Michael D" Subject: Re: [edk2-devel] Recent changes to EsrtFmp causing ASSERTs Thread-Topic: [edk2-devel] Recent changes to EsrtFmp causing ASSERTs Thread-Index: AQHVd9BmZ7BYSTaR0kKV5R5yzoXF+qdFJ8wAgAAIjoCAANSNAA== Date: Tue, 1 Oct 2019 17:30:58 +0000 Message-ID: References: <19681.1569876496768913642@groups.io> <709B20DB-2303-422B-9244-C7F9DD6CF535@intel.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.22.254.139] MIME-Version: 1.0 Return-Path: michael.d.kinney@intel.com Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_E92EE9817A31E24EB0585FDF735412F5B9DCE5A4ORSMSX113amrcor_" --_000_E92EE9817A31E24EB0585FDF735412F5B9DCE5A4ORSMSX113amrcor_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Jason, I believe the logic to check for uniqueness of FMP Descriptor is correct. The UEFI Spec has 2 structs with HardwareInstance. One is FMP Descriptor = and the other is UEFI Capsule for FMP. The HardwareInstance in FMP must be unique and not 0 unless there is a gua= rantee there is only one instance. /// /// An optional number to identify the unique hardware instance within t= he system for /// devices that may have multiple instances (Example: a plug in pci net= work card). This /// number must be unique within the namespace of the ImageTypeId GUID a= nd /// ImageIndex. For FMP instances that have multiple descriptors for a s= ingle /// hardware instance, all descriptors must have the same HardwareInstan= ce value. /// This number must be consistent between boots and should be based on = some sort of /// hardware identified unique id (serial number, etc) whenever possible= . If a hardware /// based number is not available the FMP provider may use some other ch= aracteristic /// such as device path, bus/dev/function, slot num, etc for generating = the /// HardwareInstance. For implementations that will never have more than= one /// instance a zero can be used. A zero means the FMP provider is not ab= le to determine a /// unique hardware instance number or a hardware instance number is not= needed. Only /// present in version 3 or higher. /// UINT64 HardwareInstance; } EFI_FIRMWARE_IMAGE_DESCRIPTOR; The UEFI Capsule one can be 0 to specify match any. /// /// The HardwareInstance to target with this update. If value is zero it= means match all /// HardwareInstances. This field allows update software to target only = a single device in /// cases where there are more than one device with the same ImageTypeId= GUID. /// This header is outside the signed data of the Authentication Info st= ructure and /// therefore can be modified without changing the Auth data. /// UINT64 UpdateHardwareInstance; } EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER; Best regards, Mike From: devel@edk2.groups.io On Behalf Of Spottswood,= Jason Sent: Monday, September 30, 2019 2:24 PM To: Rothman, Michael A ; devel@edk2.groups.io Subject: Re: [edk2-devel] Recent changes to EsrtFmp causing ASSERTs Cut-n-paste problem. My apologies. Copying again in plain text: // // Check to see of FmpImageInfoBuf GUID/HardwareInstance is unique // for (Index =3D 0; Index < *NumberOfDescriptors; Index++) { if (CompareGuid (&HardwareInstances[Index].ImageTypeGuid, &FmpImageInf= oBuf->ImageTypeId)) { if (HardwareInstances[Index].HardwareInstance =3D=3D FmpHardwareInst= ance) { DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Duplicate firmware image descrip= tor with GUID %g HardwareInstance:0x%x\n", &FmpImageInfoBuf->ImageTypeId, F= mpHardwareInstance)); ASSERT ( !CompareGuid (&HardwareInstances[Index].ImageTypeGuid, &FmpImage= InfoBuf->ImageTypeId) || HardwareInstances[Index].HardwareInstance !=3D FmpHardwareInstan= ce ); return EFI_UNSUPPORTED; } } } -Jason From: Rothman, Michael A > Sent: Monday, September 30, 2019 3:53 PM To: devel@edk2.groups.io; Spottswood, Jason <= jason.spottswood@hpe.com> Subject: Re: [edk2-devel] Recent changes to EsrtFmp causing ASSERTs Jason, Agreed - though that image you sent was challenging for these old e= yes. Black on dark grey? Ack! Thanks, Michael A. Rothman --------------------------------------------------------------- Let no excuse be a barrier to your success. On Sep 30, 2019, at 1:48 PM, "jason.spottswood@hpe.com" > = wrote: In EsrtFmp.c, function CreateEsrtEntry, line 196, the code asserts if the = FMP image hardware instance matches that of an existing instance. This is = fine if the hardware instance is supported. The field is optional though. = In the UEFI spec, "a zero hardware instance means the FMP provider is not = able to determine a unique hardware instance number or a hardware instance = number is not needed." The code below needs to also check if the hardware = instance is supported (by comparing it to zero) before checking it against = existing entries. // // Check to see of FmpImageInfoBuf GUID/HardwareInstance is unique // for (Index =3D 0; Index < *NumberOfDescriptors; Index++) { if (CompareGuid (&HardwareInstances[Index].ImageTypeGuid, &FmpImageInf= oBuf->ImageTypeId)) { if (HardwareInstances[Index].HardwareInstance =3D=3D FmpHardwareInst= ance) { DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Duplicate firmware image descrip= tor with GUID %g HardwareInstance:0x%x\n", &FmpImageInfoBuf->ImageTypeId, F= mpHardwareInstance)); ASSERT ( !CompareGuid (&HardwareInstances[Index].ImageTypeGuid, &FmpImage= InfoBuf->ImageTypeId) || HardwareInstances[Index].HardwareInstance !=3D FmpHardwareInstan= ce ); return EFI_UNSUPPORTED; } } } --_000_E92EE9817A31E24EB0585FDF735412F5B9DCE5A4ORSMSX113amrcor_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable

H= i Jason,

<= o:p> 

I= believe the logic to check for uniqueness of FMP Descriptor is correct.

<= o:p> 

T= he UEFI Spec has 2 structs with HardwareInstance.  One is FMP Descriptor and the other is UEFI Capsule for FMP.

<= o:p> 

T= he HardwareInstance in FMP must be unique and n= ot 0 unless there is a guarantee there is only one instance.

<= o:p> 

<= span style=3D"mso-spacerun:yes">  ///

<= span style=3D"mso-spacerun:yes">  /// An optional number to identify the unique hardware instance wit= hin the system for

<= span style=3D"mso-spacerun:yes">  /// devices that may have multiple instances (Example: a plug in pci network card). This

<= span style=3D"mso-spacerun:yes">  /// number must be unique within the namespace of the ImageTypeId GUID and

<= span style=3D"mso-spacerun:yes">  /// ImageIndex. For FMP instances tha= t have multiple descriptors for a single

<= span style=3D"mso-spacerun:yes">  /// hardware instance, all descriptors must have the same HardwareInstance value.

<= span style=3D"mso-spacerun:yes">  /// This number must be consistent between boots and should be base= d on some sort of

<= span style=3D"mso-spacerun:yes">  /// hardware identified unique id (serial number, etc) whenever possible. If a hardware

<= span style=3D"mso-spacerun:yes">  /// based number is not available the FMP provider may use some oth= er characteristic

<= span style=3D"mso-spacerun:yes">  /// such as device path, bus/dev/function, slot num, etc for generating the

<= span style=3D"mso-spacerun:yes">  /// HardwareInstance. For implementat= ions that will never have more than one

<= span style=3D"mso-spacerun:yes">  /// instance a zero can be used. A zero means the FMP provider is n= ot able to determine a

<= span style=3D"mso-spacerun:yes">  /// unique hardware instance number or a hardware instance number i= s not needed. Only

<= span style=3D"mso-spacerun:yes">  /// present in version 3 or higher.

<= span style=3D"mso-spacerun:yes">  ///

<= span style=3D"mso-spacerun:yes">  UINT64    &nbs= p;            &= nbsp;         HardwareInstance;

}= EFI_FIRMWARE_IMAGE_DESCRIPTOR;

<= o:p> 

T= he UEFI Capsule one can be 0 to specify match any.

<= o:p> 

<= span style=3D"mso-spacerun:yes">  ///

<= span style=3D"mso-spacerun:yes">  /// The HardwareInstance to target wi= th this update. If value is zero it means match all

<= span style=3D"mso-spacerun:yes">  /// HardwareInstances. This field all= ows update software to target only a single device in

<= span style=3D"mso-spacerun:yes">  /// cases where there are more than one device with the same ImageTypeId GUID.

<= span style=3D"mso-spacerun:yes">  /// This header is outside the signed data of the Authentication In= fo structure and

<= span style=3D"mso-spacerun:yes">  /// therefore can be modified without changing the Auth data.<= /o:p>

<= span style=3D"mso-spacerun:yes">  ///

<= span style=3D"mso-spacerun:yes">  UINT64   UpdateHardwareInstance;

}= EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER;

<= o:p> 

B= est regards,

<= o:p> 

M= ike

<= o:p> 

<= o:p> 

From: devel@edk2.groups.io <devel@edk2.groups.io= > On Behalf Of Spottswood, Jason
Sent: Monday, September 30, 2019 2:24 PM
To: Rothman, Michael A <michael.a.rothman@intel.com>; devel@e= dk2.groups.io
Subject: Re: [edk2-devel] Recent changes to EsrtFmp causing ASSERTs=

 

Cut-n-paste problem. My apologies. 

 

Copying again in plain text:

 

  //

  // Check to see of FmpImageInfoBuf GUID/HardwareInsta= nce is unique

  //

  for (Index =3D 0; Index < *NumberOfDescriptors; In= dex++) {

    if (CompareGuid (&HardwareInstances[I= ndex].ImageTypeGuid, &FmpImageInfoBuf->ImageTypeId)) {

      if (HardwareInstances[Index].= HardwareInstance =3D=3D FmpHardwareInstance) {

        DEBUG ((DEBUG_ERR= OR, "EsrtFmpDxe: Duplicate firmware image descriptor with GUID %g Hard= wareInstance:0x%x\n", &FmpImageInfoBuf->ImageTypeId, FmpHardwar= eInstance));

        ASSERT (

          !Comp= areGuid (&HardwareInstances[Index].ImageTypeGuid, &FmpImageInfoBuf-= >ImageTypeId) ||

          Hardw= areInstances[Index].HardwareInstance !=3D FmpHardwareInstance

          );

        return EFI_UNSUPP= ORTED;

      }

    }

  }

 

-Jason

 

From: Rothman,= Michael A <michael.a.rot= hman@intel.com>
Sent: Monday, September 30, 2019 3:53 PM
To: devel@edk2.groups.io; Spottswood, Jason <jason.= spottswood@hpe.com>
Subject: Re: [edk2-devel] Recent changes to EsrtFmp causing ASSERTs=

 

Jason, Agreed - thou= gh that image you sent was challenging for these old eyes. Black on dark gr= ey? Ack!

Thanks,

Michael A. Rothman

---------------------------------------------------= ------------

Let no excuse be a barrier to your success.


On Sep 30, 2019, at 1:48 PM, "jason.spottswood@hpe.com" <jason.spottswood@hpe.com> wrote:

In EsrtFmp.c, functi= on CreateEsrtEntry, line 196, the code asserts if the FMP image hardware in= stance matches that of an existing instance.  This is fine if the hard= ware instance is supported.  The field is optional though.  In the UEFI spec, "a zero hardware instance m= eans the FMP provider is not able to determine a unique hardware instance n= umber or a hardware instance number is not needed."  The code bel= ow needs to also check if the hardware instance is supported (by comparing it to zero) before checking it against existing entries.

  //
  // Ch=
eck to see of FmpImageInfoBuf GUID/HardwareInstance is unique
  //
  for (=
Index =3D 0; Index < *NumberOfDescriptors; Index++) {=

  =
  if (CompareGuid (&HardwareInstances[Index].ImageTypeGuid,=
 &FmpImageInfoBuf->ImageTypeId)) {
  =
    if (HardwareInstances[Index].HardwareInstance =3D=3D Fmp=
HardwareInstance) {
  =
      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe=
: Duplicate firmware image descriptor with GUID %g HardwareInstance:0x%x\n&=
quot;, &FmpImageInfoBuf->ImageTypeId, FmpHardwareInstance));
  =
      ASSERT (
  =
        !CompareGuid (&Hardwa=
reInstances[Index].ImageTypeGuid, &FmpImageInfoBuf->ImageTypeId) ||<=
o:p>
  =
        HardwareInstances[Index].Hardwar=
eInstance !=3D FmpHardwareInstance
  =
        );
  =
      return EFI_UNSUPPORTED;

  =
    }


  =
  }


  }

--_000_E92EE9817A31E24EB0585FDF735412F5B9DCE5A4ORSMSX113amrcor_--