Re: [PATCH 3/4] NetworkPkg/TlsAuthConfigDxe: fix TlsCaCertificate attributes retrieval

["Fu, Siyuan" <siyuan.fu@intel.com>]

The patch is ok with me.
Reviewed-by: Fu Siyuan <siyuan.fu@intel.com>


> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, August 17, 2018 10:36 PM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Li,
> Songpeng <songpeng.li@intel.com>
> Subject: [PATCH 3/4] NetworkPkg/TlsAuthConfigDxe: fix TlsCaCertificate
> attributes retrieval
> 
> Per spec, the GetVariable() runtime service is not required to populate
> (*Attributes) on output when it fails with EFI_BUFFER_TOO_SMALL.
> 
> Therefore we have to fetch the full contents of the TlsCaCertificate
> variable temporarily, just so we can (a) get the current attributes, and
> (b) add EFI_VARIABLE_APPEND_WRITE to them for the subsequent SetVariable()
> call.
> 
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Cc: Songpeng Li <songpeng.li@intel.com>
> Reported-by: Songpeng Li <songpeng.li@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1090
> Fixes: b90c335fbbb674470fbf09601cc522bf61564c30
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>     Tested via loading the same CA cert .pem file twice in a row, using
> the
>     HII form, first without any pre-existent TlsCaCertificate variable.
> 
>     Songpeng, can you please test this patch as well, and confirm if it
>     works on your end? Thanks!
> 
>  NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c | 27 +++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c
> b/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c
> index 7259c5e82f61..0780b03bbab4 100644
> --- a/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c
> +++ b/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c
> @@ -663,6 +663,7 @@ EnrollX509toVariable (
>    EFI_SIGNATURE_LIST                *CACert;
>    EFI_SIGNATURE_DATA                *CACertData;
>    VOID                              *Data;
> +  VOID                              *CurrentData;
>    UINTN                             DataSize;
>    UINTN                             SigDataSize;
>    UINT32                            Attr;
> @@ -674,6 +675,7 @@ EnrollX509toVariable (
>    CACert        = NULL;
>    CACertData    = NULL;
>    Data          = NULL;
> +  CurrentData   = NULL;
>    Attr          = 0;
> 
>    Status = ReadFileContent (
> @@ -716,11 +718,30 @@ EnrollX509toVariable (
>    Status = gRT->GetVariable(
>                    VariableName,
>                    &gEfiTlsCaCertificateGuid,
> -                  &Attr,
> +                  NULL,
>                    &DataSize,
>                    NULL
>                    );
>    if (Status == EFI_BUFFER_TOO_SMALL) {
> +    //
> +    // Per spec, we have to fetch the variable's contents, even though
> we're
> +    // only interested in the variable's attributes.
> +    //
> +    CurrentData = AllocatePool (DataSize);
> +    if (CurrentData == NULL) {
> +      Status = EFI_OUT_OF_RESOURCES;
> +      goto ON_EXIT;
> +    }
> +    Status = gRT->GetVariable(
> +                    VariableName,
> +                    &gEfiTlsCaCertificateGuid,
> +                    &Attr,
> +                    &DataSize,
> +                    CurrentData
> +                    );
> +    if (EFI_ERROR (Status)) {
> +      goto ON_EXIT;
> +    }
>      Attr |= EFI_VARIABLE_APPEND_WRITE;
>    } else if (Status == EFI_NOT_FOUND) {
>      Attr = TLS_AUTH_CONFIG_VAR_BASE_ATTR;
> @@ -751,6 +772,10 @@ ON_EXIT:
>      FreePool (Data);
>    }
> 
> +  if (CurrentData != NULL) {
> +    FreePool (CurrentData);
> +  }
> +
>    if (X509Data != NULL) {
>      FreePool (X509Data);
>    }
> --
> 2.14.1.3.gb7cf6e02401b
>