Re: [edk2-devel] [PATCH 1/1] MdePkg:Updated the comments of EFI_FIRMWARE_MANAGEMENT_PROTOCOL

["Michael D Kinney" <michael.d.kinney@intel.com>]

Hi Guo,

One comment below.

Mike


> -----Original Message-----
> From: Xu, GuoX <guox.xu@intel.com>
> Sent: Monday, January 29, 2024 11:18 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; gaoliming
> <gaoliming@byosoft.com.cn>; devel@edk2.groups.io
> Cc: Liu, Zhiguang <zhiguang.liu@intel.com>
> Subject: RE: [edk2-devel] [PATCH 1/1] MdePkg:Updated the comments of
> EFI_FIRMWARE_MANAGEMENT_PROTOCOL
> 
> Hi Michael,
> 
> Thanks for your suggestions.
> 
> 1. In edk2 repo there are two implementations for Firmware Management
> Protocol.
>   1.1 In edk2\FmpDevicePkg\FmpDxe\FmpDxe.c, function GetTheImageInfo():
>     //
>     // Check the buffer size
>     // NOTE: Check this first so caller can get the necessary memory
> size it must allocate.
>     //
>     if (*ImageInfoSize < (sizeof (EFI_FIRMWARE_IMAGE_DESCRIPTOR))) {
>       *ImageInfoSize = sizeof (EFI_FIRMWARE_IMAGE_DESCRIPTOR);
>       DEBUG ((DEBUG_VERBOSE, "FmpDxe(%s): GetImageInfo() - ImageInfoSize
> is to small.\n", mImageIdName));
>       Status = EFI_BUFFER_TOO_SMALL;
>       goto cleanup;
>     }
> 
>     //
>     // Confirm that buffer isn't null
>     //
>     if (  (ImageInfo == NULL) || (DescriptorVersion == NULL) ||
> (DescriptorCount == NULL) || (DescriptorSize == NULL)
>        || (PackageVersion == NULL))
>     {
>       DEBUG ((DEBUG_ERROR, "FmpDxe(%s): GetImageInfo() - Pointer
> Parameter is NULL.\n", mImageIdName));
>       Status = EFI_INVALID_PARAMETER;
>       goto cleanup;
>     }
>     In the line 497 to 498, it is missed the judgment of
> PackageVersionName but there is a comment in line 529 " // Do not update
> PackageVersionName since it is not supported in this instance."
>     So, do we need to add the judgment of PackageVersionName in this
> function?

Yes.  The check needs to be added for PackageVersionName being NULL for
this protocol service to be conformant with the UEFI Specification.

The fact that this specific implementation does not update PackageVersionName
contents does not change what a conformance test that checks the behavior
of a NULL parameter value would conclude that the service does not follow
the spec.

> 
>   1.2 In
> edk2\SignedCapsulePkg\Universal\SystemFirmwareUpdate\SystemFirmwareCommo
> nDxe.c, function FmpGetImageInfo():
>          if (ImageInfoSize == NULL) {
>            return EFI_INVALID_PARAMETER;
>          }
> 
>          if (*ImageInfoSize < sizeof (EFI_FIRMWARE_IMAGE_DESCRIPTOR) *
> SystemFmpPrivate->DescriptorCount) {
>            *ImageInfoSize = sizeof (EFI_FIRMWARE_IMAGE_DESCRIPTOR) *
> SystemFmpPrivate->DescriptorCount;
>            return EFI_BUFFER_TOO_SMALL;
>         }
> 
>         if ((ImageInfo == NULL) ||
>           (DescriptorVersion == NULL) ||
>           (DescriptorCount == NULL) ||
>           (DescriptorSize == NULL) ||
>           (PackageVersion == NULL) ||
>           (PackageVersionName == NULL))
>        {
>          return EFI_INVALID_PARAMETER;
>        }
>     The implementation already included the required code changes.
> 
>  2. In edk2-platforms repo there is only one implementation for Firmware
> Management Protocol.
>      In the file MicrocodeFmp.c, function FmpGetImageInfo():
>      If (ImageInfoSize == NULL) {
>        return EFI_INVALID_PARAMETER;
>      }
>      if (*ImageInfoSize < sizeof(EFI_FIRMWARE_IMAGE_DESCRIPTOR) *
> MicrocodeFmpPrivate->DescriptorCount) {
>        *ImageInfoSize = sizeof(EFI_FIRMWARE_IMAGE_DESCRIPTOR) *
> MicrocodeFmpPrivate->DescriptorCount;
>        return EFI_BUFFER_TOO_SMALL;
>      }
>      if (ImageInfo == NULL ||
>         DescriptorVersion == NULL ||
>         DescriptorCount == NULL ||
>         DescriptorSize == NULL ||
>         PackageVersion == NULL ||
>         PackageVersionName == NULL) {
>       return EFI_INVALID_PARAMETER;
>     }
>    The implementation already included the required code changes.
> 
> Thanks
> Xu Guo
> 
> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Tuesday, January 23, 2024 7:08 AM
> To: Xu, GuoX <guox.xu@intel.com>; gaoliming <gaoliming@byosoft.com.cn>;
> devel@edk2.groups.io
> Cc: Liu, Zhiguang <zhiguang.liu@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: RE: [edk2-devel] [PATCH 1/1] MdePkg:Updated the comments of
> EFI_FIRMWARE_MANAGEMENT_PROTOCOL
> 
> Hi Guo,
> 
> Thank you for starting this task to update Firmware Management Protocol.
> 
> There are additional tasks to make sure all required code changes are
> also implemented for a specification update like this.
> 
> * Create a TianoCore Bugzilla that describes the specification update
>   needed with links to the public documents with the required
>   specification update.
> 
> * Review/update edk2 repo include files.
> 
> * Review/update function headers of the implementations of the
>   Firmware Management Protocol in the edk2 repo.
> 
> * Review/update logic in implementations of the Firmware
>   Management Protocol in the edk2 repo to make sure all error
>   return conditions are checked.
> 
> * Review/update function headers of the implementations of the
>   Firmware Management Protocol in the edk2-platforms repo.
> 
> * Review/update logic in implementations of the Firmware
>   Management Protocol in the edk2-platforms repo to make sure
>   all error return conditions are checked.
> 
> * Review/update test cases edk2-test repo for the UEFI SCTs for
>   these error return conditions.
> 
> 
> The "REF" in the commit message should be a link to the TianoCore
> Bugzilla.  The reference to the specification is also required, but
> should be in the text of the commit message.
> 
> Thanks,
> 
> Mike
> 
> 
> > -----Original Message-----
> > From: Xu, GuoX <guox.xu@intel.com>
> > Sent: Sunday, January 21, 2024 9:37 PM
> > To: gaoliming <gaoliming@byosoft.com.cn>; devel@edk2.groups.io
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Liu, Zhiguang
> > <zhiguang.liu@intel.com>
> > Subject: RE: [edk2-devel] [PATCH 1/1] MdePkg:Updated the comments of
> > EFI_FIRMWARE_MANAGEMENT_PROTOCOL
> >
> > Hi Liming,
> >   I created a PR: https://github.com/tianocore/edk2/pull/5182, could
> > you help push it ?
> >
> > Thanks
> > Xu Guo
> >
> > -----Original Message-----
> > From: gaoliming <gaoliming@byosoft.com.cn>
> > Sent: Tuesday, January 16, 2024 10:20 PM
> > To: devel@edk2.groups.io; Xu, GuoX <guox.xu@intel.com>
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Liu, Zhiguang
> > <zhiguang.liu@intel.com>
> > Subject: 回复: [edk2-devel] [PATCH 1/1] MdePkg:Updated the comments of
> > EFI_FIRMWARE_MANAGEMENT_PROTOCOL
> >
> > I am OK for this change. Reviewed-by: Liming Gao
> > <gaoliming@byosoft.com.cn>
> >
> > > -----邮件原件-----
> > > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Xu, GuoX
> > > 发送时间: 2024年1月9日 17:24
> > > 收件人: devel@edk2.groups.io
> > > 抄送: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> > > <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>
> > > 主题: Re: [edk2-devel] [PATCH 1/1] MdePkg:Updated the comments of
> > > EFI_FIRMWARE_MANAGEMENT_PROTOCOL
> > >
> > > Hi all, any comments about this patch?
> > >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of GuoX
> > > Xu
> > > Sent: Monday, December 25, 2023 9:21 AM
> > > To: devel@edk2.groups.io
> > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> > > <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>;
> > > Li, Yi1 <yi1.li@intel.com>
> > > Subject: [edk2-devel] [PATCH 1/1] MdePkg:Updated the comments of
> > > EFI_FIRMWARE_MANAGEMENT_PROTOCOL
> > >
> > > 1.For EFI_FIRMWARE_MANAGEMENT_PROTOCOL.GetImage():
> > > Add the following sentence at the end of the Image parameter
> > description.
> > > "May be NULL with a zero ImageSize in order to determine the size of
> > > the buffer needed".
> > >
> > > Modify the description of "EFI_INVALID_PARAMETER" return code as
> > > "The ImageSize is not too small and Image is NULL."
> > >
> > > 2.For EFI_FIRMWARE_MANAGEMENT_PROTOCOL.GetImageInfo():
> > > Add the following sentence at the end of the ImageInfo parameter
> > > description."May be NULL with a zero ImageInfoSize in order to
> > > determine the size of the buffer needed".
> > >
> > > Modify the description of "EFI_INVALID_PARAMETER" return code as
> > > "The ImageInfoSize is not too small and Image is NULL." and add new
> > > descriptions for "EFI_INVALID_PARAMETER" return code.
> > >
> > > REF: UEFI Spec 2.7B Chapter 23.1.
> > >
> > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > > Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> > > Cc: Yi Li <yi1.li@intel.com>
> > > Signed-off-by: GuoX Xu <guox.xu@intel.com>
> > > ---
> > >  MdePkg/Include/Protocol/FirmwareManagement.h | 13 +++++++++++--
> > >  1 file changed, 11 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/MdePkg/Include/Protocol/FirmwareManagement.h
> > > b/MdePkg/Include/Protocol/FirmwareManagement.h
> > > index f37067df3455..93c8b7658e1a 100644
> > > --- a/MdePkg/Include/Protocol/FirmwareManagement.h
> > > +++ b/MdePkg/Include/Protocol/FirmwareManagement.h
> > > @@ -293,6 +293,8 @@ EFI_STATUS
> > >                                       to contain the image(s)
> > > information if the buffer was too small.
> > >    @param[in, out] ImageInfo          A pointer to the buffer in
> which
> > > firmware places the current image(s)
> > >                                       information. The information
> > > is an array of EFI_FIRMWARE_IMAGE_DESCRIPTORs.
> > > +                                     May be NULL with a zero
> > > ImageInfoSize in order to determine the size of the
> > > +                                     buffer needed.
> > >    @param[out]     DescriptorVersion  A pointer to the location in
> > which
> > > firmware returns the version number
> > >                                       associated with the
> > > EFI_FIRMWARE_IMAGE_DESCRIPTOR.
> > >    @param[out]     DescriptorCount    A pointer to the location in
> > > which firmware returns the number of @@ -313,7 +315,12 @@ EFI_STATUS
> > >    @retval EFI_SUCCESS                The device was successfully
> > > updated with the new image.
> > >    @retval EFI_BUFFER_TOO_SMALL       The ImageInfo buffer was too
> > > small. The current buffer size
> > >                                       needed to hold the image(s)
> > > information is returned in ImageInfoSize.
> > > -  @retval EFI_INVALID_PARAMETER      ImageInfoSize is NULL.
> > > +  @retval EFI_INVALID_PARAMETER      ImageInfoSize is not too small
> > > and ImageInfo is NULL.
> > > +  @retval EFI_INVALID_PARAMETER      ImageInfoSize is non-zero and
> > > DescriptorVersion is NULL.
> > > +  @retval EFI_INVALID_PARAMETER      ImageInfoSize is non-zero and
> > > DescriptorCount is NULL.
> > > +  @retval EFI_INVALID_PARAMETER      ImageInfoSize is non-zero and
> > > DescriptorSize is NULL.
> > > +  @retval EFI_INVALID_PARAMETER      ImageInfoSize is non-zero and
> > > PackageVersion is NULL.
> > > +  @retval EFI_INVALID_PARAMETER      ImageInfoSize is non-zero and
> > > PackageVersionName is NULL.
> > >    @retval EFI_DEVICE_ERROR           Valid information could not be
> > > returned. Possible corrupted image.
> > >
> > >  **/
> > > @@ -340,6 +347,8 @@ EFI_STATUS
> > >    @param[in]      ImageIndex     A unique number identifying the
> > > firmware image(s) within the device.
> > >                                   The number is between 1 and
> > > DescriptorCount.
> > >    @param[out]     Image          Points to the buffer where the
> > > current image is copied to.
> > > +                                 May be NULL with a zero
> > > ImageSize in order to determine the size of the
> > > +                                 buffer needed.
> > >    @param[in, out] ImageSize      On entry, points to the size of
> the
> > > buffer pointed to by Image, in bytes.
> > >                                   On return, points to the length of
> > > the image, in bytes.
> > >
> > > @@ -347,7 +356,7 @@ EFI_STATUS
> > >    @retval EFI_BUFFER_TOO_SMALL   The buffer specified by ImageSize
> is
> > > too small to hold the
> > >                                   image. The current buffer size
> > > needed to hold the image is returned
> > >                                   in ImageSize.
> > > -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
> > > +  @retval EFI_INVALID_PARAMETER  The ImageSize is not too small and
> > > Image is NULL.
> > >    @retval EFI_NOT_FOUND          The current image is not copied to
> > > the buffer.
> > >    @retval EFI_UNSUPPORTED        The operation is not supported.
> > >    @retval EFI_SECURITY_VIOLATION The operation could not be
> > > performed due to an authentication failure.
> > > --
> > > 2.29.2.windows.3
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > > 
> > >
> >
> >



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114812): https://edk2.groups.io/g/devel/message/114812
Mute This Topic: https://groups.io/mt/103898765/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-