[PATCH v1 0/3] Fix possible uninitialized uses

[PATCH v1 0/3] Fix possible uninitialized uses

[Sergei Dmitrouk]

Compiling for IA32 target with gcc-5.5.0 emits "maybe-uninitialized" warnings.
Compilation command: build -a IA32 -p OvmfPkg/OvmfPkgIa32.dsc -t GCC49

Unlike other cases mentioned in
https://bugzilla.tianocore.org/show_bug.cgi?id=3228
these seem to be actual issues in the code.  Read patches for specifics.

Sergei Dmitrouk (3):
  ShellPkg/HttpDynamicCommand: Fix possible uninitialized use
  MdeModulePkg/PciBusDxe: Fix possible uninitialized use
  CryptoPkg/BaseCryptLib: Fix possible uninitialized use

 CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c     | 1 +
 CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c | 1 +
 MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c             | 5 +++++
 ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c   | 1 +
 4 files changed, 8 insertions(+)

-- 
2.17.6

[PATCH v1 1/3] ShellPkg/HttpDynamicCommand: Fix possible uninitialized use

[Sergei Dmitrouk]

`Status` can be used uninitialized:

    /* Evaluates to FALSE */
    if (ShellGetExecutionBreakFlag ()) {
        Status = EFI_ABORTED;
        break;
    }

    /* Evaluates to FALSE */
    if (!Context->ContentDownloaded && !Context->ResponseToken.Event) {
        Status = ...;
        ASSERT_EFI_ERROR (Status);
    } else {
        ResponseMessage.Data.Response = NULL;
    }

    /* UNINITIALIZED USE */
    if (EFI_ERROR (Status)) {
        break;
    }

Cc: Ray Ni <ray.ni@intel.com>
Cc: Zhichao Gao <zhichao.gao@intel.com>
Signed-off-by: Sergei Dmitrouk <sergei@posteo.net>
---
 ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c b/ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c
index 3735a4a7e645..7b9b2d238015 100644
--- a/ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c
+++ b/ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c
@@ -1524,6 +1524,7 @@ GetResponse (
   Context->ResponseToken.Message = &ResponseMessage;
   Context->ContentLength = 0;
   Context->Status = REQ_OK;
+  Status = EFI_SUCCESS;
   MsgParser = NULL;
   ResponseData.StatusCode = HTTP_STATUS_UNSUPPORTED_STATUS;
   ResponseMessage.Data.Response = &ResponseData;
-- 
2.17.6

[PATCH v1 2/3] MdeModulePkg/PciBusDxe: Fix possible uninitialized use

[Sergei Dmitrouk]

If the function gets invalid value for the `ResizableBarOp` parameter
and asserts are disabled, `Bit` can be used uninitialized.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Sergei Dmitrouk <sergei@posteo.net>
---
 MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
index 6bba28367165..de601713a53b 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
@@ -1781,6 +1781,11 @@ PciProgramResizableBar (
     } else if (ResizableBarOp == PciResizableBarMin) {
       Bit = LowBitSet64(Capabilities);
     } else {
+      //
+      // Set Bit to avoid uninitialized use when built without assertions.
+      //
+      Bit = 0;
+
       ASSERT ((ResizableBarOp == PciResizableBarMax) || (ResizableBarOp == PciResizableBarMin));
     }
 
-- 
2.17.6

[PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix possible uninitialized use

[Sergei Dmitrouk]

`Result` can be used uninitialized in both functions after following
either first or second `goto` statement.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
Cc: Guomin Jiang <guomin.jiang@intel.com>
Signed-off-by: Sergei Dmitrouk <sergei@posteo.net>
---
 CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c     | 1 +
 CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
index 4009d37d5f91..0b2960f06c4c 100644
--- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
+++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
@@ -82,6 +82,7 @@ RsaPssVerify (
   EVP_PKEY_CTX *KeyCtx;
   CONST EVP_MD  *HashAlg;
 
+  Result = FALSE;
   EvpRsaKey = NULL;
   EvpVerifyCtx = NULL;
   KeyCtx = NULL;
diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
index b66b6f7296ad..ece765f9ae0a 100644
--- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
+++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
@@ -97,6 +97,7 @@ RsaPssSign (
   EVP_PKEY_CTX          *KeyCtx;
   CONST EVP_MD          *HashAlg;
 
+  Result = FALSE;
   EvpRsaKey = NULL;
   EvpVerifyCtx = NULL;
   KeyCtx = NULL;
-- 
2.17.6

Re: [PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix possible uninitialized use

[Yao, Jiewen]

Hi Sergei
Thank you very much for the fix.
Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>

I am a little surprised why it is not caught before. It is an obvious logic issue.

Do you think we can do anything on CI, to catch it during pre-check-in in the future?
I just feel it is burden to make it post-check-in fix.


Thank you
Yao Jiewen

> -----Original Message-----
> From: Sergei Dmitrouk <sergei@posteo.net>
> Sent: Friday, May 14, 2021 8:17 PM
> To: devel@edk2.groups.io
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
> Lu, XiaoyuX <xiaoyux.lu@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>
> Subject: [PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix possible uninitialized use
> 
> `Result` can be used uninitialized in both functions after following
> either first or second `goto` statement.
> 
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
> Cc: Guomin Jiang <guomin.jiang@intel.com>
> Signed-off-by: Sergei Dmitrouk <sergei@posteo.net>
> ---
>  CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c     | 1 +
>  CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> index 4009d37d5f91..0b2960f06c4c 100644
> --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> @@ -82,6 +82,7 @@ RsaPssVerify (
>    EVP_PKEY_CTX *KeyCtx;
>    CONST EVP_MD  *HashAlg;
> 
> +  Result = FALSE;
>    EvpRsaKey = NULL;
>    EvpVerifyCtx = NULL;
>    KeyCtx = NULL;
> diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> index b66b6f7296ad..ece765f9ae0a 100644
> --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> @@ -97,6 +97,7 @@ RsaPssSign (
>    EVP_PKEY_CTX          *KeyCtx;
>    CONST EVP_MD          *HashAlg;
> 
> +  Result = FALSE;
>    EvpRsaKey = NULL;
>    EvpVerifyCtx = NULL;
>    KeyCtx = NULL;
> --
> 2.17.6

Re: [edk2-devel] [PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix possible uninitialized use

[Sergei Dmitrouk]

Hello Jiewen,

I get the error only for GCC49 and not for GCC5 toolchain.  CI uses GCC5.

So I compared build commands and this seems to depend on LTO.  Adding `-flto`
impedes compiler's ability to detect such simple issues.

I've found relevant bug report, there is even fix suggestion from last month:

    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90844

Regards,
Sergei

On Sat, May 15, 2021 at 12:30:44AM +0000, Yao, Jiewen wrote:
> Hi Sergei
> Thank you very much for the fix.
> Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>
>
> I am a little surprised why it is not caught before. It is an obvious logic issue.
>
> Do you think we can do anything on CI, to catch it during pre-check-in in the future?
> I just feel it is burden to make it post-check-in fix.
>
>
> Thank you
> Yao Jiewen
>
> > -----Original Message-----
> > From: Sergei Dmitrouk <sergei@posteo.net>
> > Sent: Friday, May 14, 2021 8:17 PM
> > To: devel@edk2.groups.io
> > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
> > Lu, XiaoyuX <xiaoyux.lu@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>
> > Subject: [PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix possible uninitialized use
> >
> > `Result` can be used uninitialized in both functions after following
> > either first or second `goto` statement.
> >
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
> > Cc: Guomin Jiang <guomin.jiang@intel.com>
> > Signed-off-by: Sergei Dmitrouk <sergei@posteo.net>
> > ---
> >  CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c     | 1 +
> >  CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c | 1 +
> >  2 files changed, 2 insertions(+)
> >
> > diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> > b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> > index 4009d37d5f91..0b2960f06c4c 100644
> > --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> > +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> > @@ -82,6 +82,7 @@ RsaPssVerify (
> >    EVP_PKEY_CTX *KeyCtx;
> >    CONST EVP_MD  *HashAlg;
> >
> > +  Result = FALSE;
> >    EvpRsaKey = NULL;
> >    EvpVerifyCtx = NULL;
> >    KeyCtx = NULL;
> > diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> > b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> > index b66b6f7296ad..ece765f9ae0a 100644
> > --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> > +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> > @@ -97,6 +97,7 @@ RsaPssSign (
> >    EVP_PKEY_CTX          *KeyCtx;
> >    CONST EVP_MD          *HashAlg;
> >
> > +  Result = FALSE;
> >    EvpRsaKey = NULL;
> >    EvpVerifyCtx = NULL;
> >    KeyCtx = NULL;
> > --
> > 2.17.6

Re: [PATCH v1 2/3] MdeModulePkg/PciBusDxe: Fix possible uninitialized use

[Ni, Ray]

How about below fix? I think it might be simpler to understand and doesn't introduce unnecessary logic to handle impossible case:
    if (ResizableBarOp == PciResizableBarMax) {
      Bit = HighBitSet64(Capabilities);
    } else {
      ASSERT (ResizableBarOp == PciResizableBarMin);
      Bit = LowBitSet64(Capabilities);
    }

> -----Original Message-----
> From: Sergei Dmitrouk <sergei@posteo.net>
> Sent: Friday, May 14, 2021 8:17 PM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Ni, Ray <ray.ni@intel.com>
> Subject: [PATCH v1 2/3] MdeModulePkg/PciBusDxe: Fix possible uninitialized
> use
> 
> If the function gets invalid value for the `ResizableBarOp` parameter
> and asserts are disabled, `Bit` can be used uninitialized.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Sergei Dmitrouk <sergei@posteo.net>
> ---
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> index 6bba28367165..de601713a53b 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> @@ -1781,6 +1781,11 @@ PciProgramResizableBar (
>      } else if (ResizableBarOp == PciResizableBarMin) {
>        Bit = LowBitSet64(Capabilities);
>      } else {
> +      //
> +      // Set Bit to avoid uninitialized use when built without assertions.
> +      //
> +      Bit = 0;
> +
>        ASSERT ((ResizableBarOp == PciResizableBarMax) || (ResizableBarOp ==
> PciResizableBarMin));
>      }
> 
> --
> 2.17.6

Re: [edk2-devel] [PATCH v1 2/3] MdeModulePkg/PciBusDxe: Fix possible uninitialized use

[Sergei Dmitrouk]

If the function gets invalid value for the `ResizableBarOp` parameter
and asserts are disabled, `Bit` can be used uninitialized.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Sergei Dmitrouk <sergei@posteo.net>
---

Notes:
    v2:
    - simplify if-statement to avoid unused branches

 MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
index 6bba28367165..4caac56f1dcd 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
@@ -1778,10 +1778,9 @@ PciProgramResizableBar (
 
     if (ResizableBarOp == PciResizableBarMax) {
       Bit = HighBitSet64(Capabilities);
-    } else if (ResizableBarOp == PciResizableBarMin) {
+    } else {
+      ASSERT (ResizableBarOp == PciResizableBarMin);
       Bit = LowBitSet64(Capabilities);
-    } else {
-      ASSERT ((ResizableBarOp == PciResizableBarMax) || (ResizableBarOp == PciResizableBarMin));
     }
 
     ASSERT (Bit >= 0);
-- 
2.17.6

回复: [edk2-devel] [PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix possible uninitialized use

[gaoliming]

Sergei:
  Yes. GCC49 is LTO disable GCC tool chain. GCC5 is LTO enable tool chain.
They both work on the different GCC version, such as gcc5, gcc6..

  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90844 mentions
-ffat-lto-objects option that can trig the warning with LTO option. Do you
try it?

  If this option works, we can update GCC5 tool chain definition in
tools_def.txt, then this issue can be detected in CI GCC5 build. 

Thanks
Liming
> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Sergei
> Dmitrouk
> 发送时间: 2021年5月15日 21:01
> 收件人: devel@edk2.groups.io; jiewen.yao@intel.com
> 抄送: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
> <xiaoyux.lu@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>
> 主题: Re: [edk2-devel] [PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix possible
> uninitialized use
> 
> Hello Jiewen,
> 
> I get the error only for GCC49 and not for GCC5 toolchain.  CI uses GCC5.
> 
> So I compared build commands and this seems to depend on LTO.  Adding
> `-flto`
> impedes compiler's ability to detect such simple issues.
> 
> I've found relevant bug report, there is even fix suggestion from last
month:
> 
>     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90844
> 
> Regards,
> Sergei
> 
> On Sat, May 15, 2021 at 12:30:44AM +0000, Yao, Jiewen wrote:
> > Hi Sergei
> > Thank you very much for the fix.
> > Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>
> >
> > I am a little surprised why it is not caught before. It is an obvious
logic issue.
> >
> > Do you think we can do anything on CI, to catch it during pre-check-in
in the
> future?
> > I just feel it is burden to make it post-check-in fix.
> >
> >
> > Thank you
> > Yao Jiewen
> >
> > > -----Original Message-----
> > > From: Sergei Dmitrouk <sergei@posteo.net>
> > > Sent: Friday, May 14, 2021 8:17 PM
> > > To: devel@edk2.groups.io
> > > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> <jian.j.wang@intel.com>;
> > > Lu, XiaoyuX <xiaoyux.lu@intel.com>; Jiang, Guomin
> <guomin.jiang@intel.com>
> > > Subject: [PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix possible
uninitialized
> use
> > >
> > > `Result` can be used uninitialized in both functions after following
> > > either first or second `goto` statement.
> > >
> > > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
> > > Cc: Guomin Jiang <guomin.jiang@intel.com>
> > > Signed-off-by: Sergei Dmitrouk <sergei@posteo.net>
> > > ---
> > >  CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c     | 1 +
> > >  CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c | 1 +
> > >  2 files changed, 2 insertions(+)
> > >
> > > diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> > > b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> > > index 4009d37d5f91..0b2960f06c4c 100644
> > > --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> > > +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> > > @@ -82,6 +82,7 @@ RsaPssVerify (
> > >    EVP_PKEY_CTX *KeyCtx;
> > >    CONST EVP_MD  *HashAlg;
> > >
> > > +  Result = FALSE;
> > >    EvpRsaKey = NULL;
> > >    EvpVerifyCtx = NULL;
> > >    KeyCtx = NULL;
> > > diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> > > b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> > > index b66b6f7296ad..ece765f9ae0a 100644
> > > --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> > > +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> > > @@ -97,6 +97,7 @@ RsaPssSign (
> > >    EVP_PKEY_CTX          *KeyCtx;
> > >    CONST EVP_MD          *HashAlg;
> > >
> > > +  Result = FALSE;
> > >    EvpRsaKey = NULL;
> > >    EvpVerifyCtx = NULL;
> > >    KeyCtx = NULL;
> > > --
> > > 2.17.6
> 
> 
> 
> 

Re: [edk2-devel] [PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix possible uninitialized use

[Ard Biesheuvel]

Please merge this fix asap. Our CI is broken because of it, and we are
in the soft freeze so we need the CI up and running to catch potential
issues before the release.

Thanks,
Ard.

On Tue, 18 May 2021 at 02:59, gaoliming <gaoliming@byosoft.com.cn> wrote:
>
> Sergei:
>   Yes. GCC49 is LTO disable GCC tool chain. GCC5 is LTO enable tool chain.
> They both work on the different GCC version, such as gcc5, gcc6..
>
>   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90844 mentions
> -ffat-lto-objects option that can trig the warning with LTO option. Do you
> try it?
>
>   If this option works, we can update GCC5 tool chain definition in
> tools_def.txt, then this issue can be detected in CI GCC5 build.
>
> Thanks
> Liming
> > -----邮件原件-----
> > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Sergei
> > Dmitrouk
> > 发送时间: 2021年5月15日 21:01
> > 收件人: devel@edk2.groups.io; jiewen.yao@intel.com
> > 抄送: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
> > <xiaoyux.lu@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>
> > 主题: Re: [edk2-devel] [PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix possible
> > uninitialized use
> >
> > Hello Jiewen,
> >
> > I get the error only for GCC49 and not for GCC5 toolchain.  CI uses GCC5.
> >
> > So I compared build commands and this seems to depend on LTO.  Adding
> > `-flto`
> > impedes compiler's ability to detect such simple issues.
> >
> > I've found relevant bug report, there is even fix suggestion from last
> month:
> >
> >     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90844
> >
> > Regards,
> > Sergei
> >
> > On Sat, May 15, 2021 at 12:30:44AM +0000, Yao, Jiewen wrote:
> > > Hi Sergei
> > > Thank you very much for the fix.
> > > Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>
> > >
> > > I am a little surprised why it is not caught before. It is an obvious
> logic issue.
> > >
> > > Do you think we can do anything on CI, to catch it during pre-check-in
> in the
> > future?
> > > I just feel it is burden to make it post-check-in fix.
> > >
> > >
> > > Thank you
> > > Yao Jiewen
> > >
> > > > -----Original Message-----
> > > > From: Sergei Dmitrouk <sergei@posteo.net>
> > > > Sent: Friday, May 14, 2021 8:17 PM
> > > > To: devel@edk2.groups.io
> > > > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> > <jian.j.wang@intel.com>;
> > > > Lu, XiaoyuX <xiaoyux.lu@intel.com>; Jiang, Guomin
> > <guomin.jiang@intel.com>
> > > > Subject: [PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix possible
> uninitialized
> > use
> > > >
> > > > `Result` can be used uninitialized in both functions after following
> > > > either first or second `goto` statement.
> > > >
> > > > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > > Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
> > > > Cc: Guomin Jiang <guomin.jiang@intel.com>
> > > > Signed-off-by: Sergei Dmitrouk <sergei@posteo.net>
> > > > ---
> > > >  CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c     | 1 +
> > > >  CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c | 1 +
> > > >  2 files changed, 2 insertions(+)
> > > >
> > > > diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> > > > b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> > > > index 4009d37d5f91..0b2960f06c4c 100644
> > > > --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> > > > +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> > > > @@ -82,6 +82,7 @@ RsaPssVerify (
> > > >    EVP_PKEY_CTX *KeyCtx;
> > > >    CONST EVP_MD  *HashAlg;
> > > >
> > > > +  Result = FALSE;
> > > >    EvpRsaKey = NULL;
> > > >    EvpVerifyCtx = NULL;
> > > >    KeyCtx = NULL;
> > > > diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> > > > b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> > > > index b66b6f7296ad..ece765f9ae0a 100644
> > > > --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> > > > +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> > > > @@ -97,6 +97,7 @@ RsaPssSign (
> > > >    EVP_PKEY_CTX          *KeyCtx;
> > > >    CONST EVP_MD          *HashAlg;
> > > >
> > > > +  Result = FALSE;
> > > >    EvpRsaKey = NULL;
> > > >    EvpVerifyCtx = NULL;
> > > >    KeyCtx = NULL;
> > > > --
> > > > 2.17.6
> >
> >
> >
> >
>
>
>
>
>
> 
>
>

Re: [edk2-devel] [PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix possible uninitialized use

[Wang, Jian J]

Ard,

Patch 1&2 haven't got r-b. I'm not sure we can merge patch 3 separately.

Regards,
Jian

> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Tuesday, May 18, 2021 3:27 PM
> To: edk2-devel-groups-io <devel@edk2.groups.io>; Liming Gao (Byosoft address)
> <gaoliming@byosoft.com.cn>
> Cc: sergei@posteo.net; Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>; Jiang, Guomin
> <guomin.jiang@intel.com>
> Subject: Re: [edk2-devel] [PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix possible
> uninitialized use
> 
> Please merge this fix asap. Our CI is broken because of it, and we are
> in the soft freeze so we need the CI up and running to catch potential
> issues before the release.
> 
> Thanks,
> Ard.
> 
> On Tue, 18 May 2021 at 02:59, gaoliming <gaoliming@byosoft.com.cn> wrote:
> >
> > Sergei:
> >   Yes. GCC49 is LTO disable GCC tool chain. GCC5 is LTO enable tool chain.
> > They both work on the different GCC version, such as gcc5, gcc6..
> >
> >   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90844 mentions
> > -ffat-lto-objects option that can trig the warning with LTO option. Do you
> > try it?
> >
> >   If this option works, we can update GCC5 tool chain definition in
> > tools_def.txt, then this issue can be detected in CI GCC5 build.
> >
> > Thanks
> > Liming
> > > -----邮件原件-----
> > > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Sergei
> > > Dmitrouk
> > > 发送时间: 2021年5月15日 21:01
> > > 收件人: devel@edk2.groups.io; jiewen.yao@intel.com
> > > 抄送: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
> > > <xiaoyux.lu@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>
> > > 主题: Re: [edk2-devel] [PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix possible
> > > uninitialized use
> > >
> > > Hello Jiewen,
> > >
> > > I get the error only for GCC49 and not for GCC5 toolchain.  CI uses GCC5.
> > >
> > > So I compared build commands and this seems to depend on LTO.  Adding
> > > `-flto`
> > > impedes compiler's ability to detect such simple issues.
> > >
> > > I've found relevant bug report, there is even fix suggestion from last
> > month:
> > >
> > >     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90844
> > >
> > > Regards,
> > > Sergei
> > >
> > > On Sat, May 15, 2021 at 12:30:44AM +0000, Yao, Jiewen wrote:
> > > > Hi Sergei
> > > > Thank you very much for the fix.
> > > > Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>
> > > >
> > > > I am a little surprised why it is not caught before. It is an obvious
> > logic issue.
> > > >
> > > > Do you think we can do anything on CI, to catch it during pre-check-in
> > in the
> > > future?
> > > > I just feel it is burden to make it post-check-in fix.
> > > >
> > > >
> > > > Thank you
> > > > Yao Jiewen
> > > >
> > > > > -----Original Message-----
> > > > > From: Sergei Dmitrouk <sergei@posteo.net>
> > > > > Sent: Friday, May 14, 2021 8:17 PM
> > > > > To: devel@edk2.groups.io
> > > > > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> > > <jian.j.wang@intel.com>;
> > > > > Lu, XiaoyuX <xiaoyux.lu@intel.com>; Jiang, Guomin
> > > <guomin.jiang@intel.com>
> > > > > Subject: [PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix possible
> > uninitialized
> > > use
> > > > >
> > > > > `Result` can be used uninitialized in both functions after following
> > > > > either first or second `goto` statement.
> > > > >
> > > > > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > > > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > > > Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
> > > > > Cc: Guomin Jiang <guomin.jiang@intel.com>
> > > > > Signed-off-by: Sergei Dmitrouk <sergei@posteo.net>
> > > > > ---
> > > > >  CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c     | 1 +
> > > > >  CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c | 1 +
> > > > >  2 files changed, 2 insertions(+)
> > > > >
> > > > > diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> > > > > b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> > > > > index 4009d37d5f91..0b2960f06c4c 100644
> > > > > --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> > > > > +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> > > > > @@ -82,6 +82,7 @@ RsaPssVerify (
> > > > >    EVP_PKEY_CTX *KeyCtx;
> > > > >    CONST EVP_MD  *HashAlg;
> > > > >
> > > > > +  Result = FALSE;
> > > > >    EvpRsaKey = NULL;
> > > > >    EvpVerifyCtx = NULL;
> > > > >    KeyCtx = NULL;
> > > > > diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> > > > > b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> > > > > index b66b6f7296ad..ece765f9ae0a 100644
> > > > > --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> > > > > +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> > > > > @@ -97,6 +97,7 @@ RsaPssSign (
> > > > >    EVP_PKEY_CTX          *KeyCtx;
> > > > >    CONST EVP_MD          *HashAlg;
> > > > >
> > > > > +  Result = FALSE;
> > > > >    EvpRsaKey = NULL;
> > > > >    EvpVerifyCtx = NULL;
> > > > >    KeyCtx = NULL;
> > > > > --
> > > > > 2.17.6
> > >
> > >
> > >
> > >
> >
> >
> >
> >
> >
> > 
> >
> >

Re: [edk2-devel] [PATCH v1 2/3] MdeModulePkg/PciBusDxe: Fix possible uninitialized use

[Wu, Hao A]

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sergei
> Dmitrouk
> Sent: Tuesday, May 18, 2021 12:23 AM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> Subject: Re: [edk2-devel] [PATCH v1 2/3] MdeModulePkg/PciBusDxe: Fix
> possible uninitialized use
> 
> If the function gets invalid value for the `ResizableBarOp` parameter and
> asserts are disabled, `Bit` can be used uninitialized.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Sergei Dmitrouk <sergei@posteo.net>
> ---
> 
> Notes:
>     v2:
>     - simplify if-statement to avoid unused branches


Hello,

Since the V1 is a patch series, I would suggest to send the whole series for V2 changes (even if other patches are unchanged).

With this handled:
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu

> 
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> index 6bba28367165..4caac56f1dcd 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> @@ -1778,10 +1778,9 @@ PciProgramResizableBar (
> 
>      if (ResizableBarOp == PciResizableBarMax) {
>        Bit = HighBitSet64(Capabilities);
> -    } else if (ResizableBarOp == PciResizableBarMin) {
> +    } else {
> +      ASSERT (ResizableBarOp == PciResizableBarMin);
>        Bit = LowBitSet64(Capabilities);
> -    } else {
> -      ASSERT ((ResizableBarOp == PciResizableBarMax) || (ResizableBarOp ==
> PciResizableBarMin));
>      }
> 
>      ASSERT (Bit >= 0);
> --
> 2.17.6
> 
> 
> 
> 
> 

Re: 回复: [edk2-devel] [PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix possible uninitialized use

[Sergei Dmitrouk]

Yes, adding `-ffat-lto-objects` makes the warning appear even with GCC 5.5.0.

Regards,
Sergei

On Tue, May 18, 2021 at 08:59:05AM +0800, gaoliming wrote:
> Sergei:
>   Yes. GCC49 is LTO disable GCC tool chain. GCC5 is LTO enable tool chain.
> They both work on the different GCC version, such as gcc5, gcc6..
> 
>   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90844 mentions
> -ffat-lto-objects option that can trig the warning with LTO option. Do you
> try it?
> 
>   If this option works, we can update GCC5 tool chain definition in
> tools_def.txt, then this issue can be detected in CI GCC5 build. 
> 
> Thanks
> Liming
> > -----邮件原件-----
> > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Sergei
> > Dmitrouk
> > 发送时间: 2021年5月15日 21:01
> > 收件人: devel@edk2.groups.io; jiewen.yao@intel.com
> > 抄送: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
> > <xiaoyux.lu@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>
> > 主题: Re: [edk2-devel] [PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix possible
> > uninitialized use
> > 
> > Hello Jiewen,
> > 
> > I get the error only for GCC49 and not for GCC5 toolchain.  CI uses GCC5.
> > 
> > So I compared build commands and this seems to depend on LTO.  Adding
> > `-flto`
> > impedes compiler's ability to detect such simple issues.
> > 
> > I've found relevant bug report, there is even fix suggestion from last
> month:
> > 
> >     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90844
> > 
> > Regards,
> > Sergei
> > 
> > On Sat, May 15, 2021 at 12:30:44AM +0000, Yao, Jiewen wrote:
> > > Hi Sergei
> > > Thank you very much for the fix.
> > > Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>
> > >
> > > I am a little surprised why it is not caught before. It is an obvious
> logic issue.
> > >
> > > Do you think we can do anything on CI, to catch it during pre-check-in
> in the
> > future?
> > > I just feel it is burden to make it post-check-in fix.
> > >
> > >
> > > Thank you
> > > Yao Jiewen
> > >
> > > > -----Original Message-----
> > > > From: Sergei Dmitrouk <sergei@posteo.net>
> > > > Sent: Friday, May 14, 2021 8:17 PM
> > > > To: devel@edk2.groups.io
> > > > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> > <jian.j.wang@intel.com>;
> > > > Lu, XiaoyuX <xiaoyux.lu@intel.com>; Jiang, Guomin
> > <guomin.jiang@intel.com>
> > > > Subject: [PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix possible
> uninitialized
> > use
> > > >
> > > > `Result` can be used uninitialized in both functions after following
> > > > either first or second `goto` statement.
> > > >
> > > > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > > Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
> > > > Cc: Guomin Jiang <guomin.jiang@intel.com>
> > > > Signed-off-by: Sergei Dmitrouk <sergei@posteo.net>
> > > > ---
> > > >  CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c     | 1 +
> > > >  CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c | 1 +
> > > >  2 files changed, 2 insertions(+)
> > > >
> > > > diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> > > > b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> > > > index 4009d37d5f91..0b2960f06c4c 100644
> > > > --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> > > > +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> > > > @@ -82,6 +82,7 @@ RsaPssVerify (
> > > >    EVP_PKEY_CTX *KeyCtx;
> > > >    CONST EVP_MD  *HashAlg;
> > > >
> > > > +  Result = FALSE;
> > > >    EvpRsaKey = NULL;
> > > >    EvpVerifyCtx = NULL;
> > > >    KeyCtx = NULL;
> > > > diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> > > > b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> > > > index b66b6f7296ad..ece765f9ae0a 100644
> > > > --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> > > > +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> > > > @@ -97,6 +97,7 @@ RsaPssSign (
> > > >    EVP_PKEY_CTX          *KeyCtx;
> > > >    CONST EVP_MD          *HashAlg;
> > > >
> > > > +  Result = FALSE;
> > > >    EvpRsaKey = NULL;
> > > >    EvpVerifyCtx = NULL;
> > > >    KeyCtx = NULL;
> > > > --
> > > > 2.17.6
> > 
> > 
> > 
> > 
> 
> 
> 
> 
> 
> 
> 
> 

回复: [edk2-devel] [PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix possible uninitialized use

[gaoliming]

Jian:
  These three patches are separate. They don't impact others. So, I think we can merge single one. 

Thanks
Liming
> -----邮件原件-----
> 发件人: Wang, Jian J <jian.j.wang@intel.com>
> 发送时间: 2021年5月18日 15:36
> 收件人: Ard Biesheuvel <ardb@kernel.org>; edk2-devel-groups-io
> <devel@edk2.groups.io>; Liming Gao (Byosoft address)
> <gaoliming@byosoft.com.cn>
> 抄送: sergei@posteo.net; Yao, Jiewen <jiewen.yao@intel.com>; Lu, XiaoyuX
> <xiaoyux.lu@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>
> 主题: RE: [edk2-devel] [PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix possible
> uninitialized use
> 
> Ard,
> 
> Patch 1&2 haven't got r-b. I'm not sure we can merge patch 3 separately.
> 
> Regards,
> Jian
> 
> > -----Original Message-----
> > From: Ard Biesheuvel <ardb@kernel.org>
> > Sent: Tuesday, May 18, 2021 3:27 PM
> > To: edk2-devel-groups-io <devel@edk2.groups.io>; Liming Gao (Byosoft
> address)
> > <gaoliming@byosoft.com.cn>
> > Cc: sergei@posteo.net; Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> > <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>; Jiang,
> Guomin
> > <guomin.jiang@intel.com>
> > Subject: Re: [edk2-devel] [PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix
> possible
> > uninitialized use
> >
> > Please merge this fix asap. Our CI is broken because of it, and we are
> > in the soft freeze so we need the CI up and running to catch potential
> > issues before the release.
> >
> > Thanks,
> > Ard.
> >
> > On Tue, 18 May 2021 at 02:59, gaoliming <gaoliming@byosoft.com.cn>
> wrote:
> > >
> > > Sergei:
> > >   Yes. GCC49 is LTO disable GCC tool chain. GCC5 is LTO enable tool
> chain.
> > > They both work on the different GCC version, such as gcc5, gcc6..
> > >
> > >   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90844 mentions
> > > -ffat-lto-objects option that can trig the warning with LTO option. Do you
> > > try it?
> > >
> > >   If this option works, we can update GCC5 tool chain definition in
> > > tools_def.txt, then this issue can be detected in CI GCC5 build.
> > >
> > > Thanks
> > > Liming
> > > > -----邮件原件-----
> > > > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Sergei
> > > > Dmitrouk
> > > > 发送时间: 2021年5月15日 21:01
> > > > 收件人: devel@edk2.groups.io; jiewen.yao@intel.com
> > > > 抄送: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
> > > > <xiaoyux.lu@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>
> > > > 主题: Re: [edk2-devel] [PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix
> possible
> > > > uninitialized use
> > > >
> > > > Hello Jiewen,
> > > >
> > > > I get the error only for GCC49 and not for GCC5 toolchain.  CI uses
> GCC5.
> > > >
> > > > So I compared build commands and this seems to depend on LTO.
> Adding
> > > > `-flto`
> > > > impedes compiler's ability to detect such simple issues.
> > > >
> > > > I've found relevant bug report, there is even fix suggestion from last
> > > month:
> > > >
> > > >     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90844
> > > >
> > > > Regards,
> > > > Sergei
> > > >
> > > > On Sat, May 15, 2021 at 12:30:44AM +0000, Yao, Jiewen wrote:
> > > > > Hi Sergei
> > > > > Thank you very much for the fix.
> > > > > Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>
> > > > >
> > > > > I am a little surprised why it is not caught before. It is an obvious
> > > logic issue.
> > > > >
> > > > > Do you think we can do anything on CI, to catch it during pre-check-in
> > > in the
> > > > future?
> > > > > I just feel it is burden to make it post-check-in fix.
> > > > >
> > > > >
> > > > > Thank you
> > > > > Yao Jiewen
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Sergei Dmitrouk <sergei@posteo.net>
> > > > > > Sent: Friday, May 14, 2021 8:17 PM
> > > > > > To: devel@edk2.groups.io
> > > > > > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> > > > <jian.j.wang@intel.com>;
> > > > > > Lu, XiaoyuX <xiaoyux.lu@intel.com>; Jiang, Guomin
> > > > <guomin.jiang@intel.com>
> > > > > > Subject: [PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix possible
> > > uninitialized
> > > > use
> > > > > >
> > > > > > `Result` can be used uninitialized in both functions after following
> > > > > > either first or second `goto` statement.
> > > > > >
> > > > > > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > > > > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > > > > Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
> > > > > > Cc: Guomin Jiang <guomin.jiang@intel.com>
> > > > > > Signed-off-by: Sergei Dmitrouk <sergei@posteo.net>
> > > > > > ---
> > > > > >  CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c     | 1 +
> > > > > >  CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c | 1 +
> > > > > >  2 files changed, 2 insertions(+)
> > > > > >
> > > > > > diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> > > > > > b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> > > > > > index 4009d37d5f91..0b2960f06c4c 100644
> > > > > > --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> > > > > > +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> > > > > > @@ -82,6 +82,7 @@ RsaPssVerify (
> > > > > >    EVP_PKEY_CTX *KeyCtx;
> > > > > >    CONST EVP_MD  *HashAlg;
> > > > > >
> > > > > > +  Result = FALSE;
> > > > > >    EvpRsaKey = NULL;
> > > > > >    EvpVerifyCtx = NULL;
> > > > > >    KeyCtx = NULL;
> > > > > > diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> > > > > > b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> > > > > > index b66b6f7296ad..ece765f9ae0a 100644
> > > > > > --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> > > > > > +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> > > > > > @@ -97,6 +97,7 @@ RsaPssSign (
> > > > > >    EVP_PKEY_CTX          *KeyCtx;
> > > > > >    CONST EVP_MD          *HashAlg;
> > > > > >
> > > > > > +  Result = FALSE;
> > > > > >    EvpRsaKey = NULL;
> > > > > >    EvpVerifyCtx = NULL;
> > > > > >    KeyCtx = NULL;
> > > > > > --
> > > > > > 2.17.6
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> > >
> > >
> > > 
> > >
> > >

Re: [edk2-devel] [PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix possible uninitialized use

[Yao, Jiewen]

OK. I suggest we merge this specific one ASAP, since it is blocking other development work.


> -----Original Message-----
> From: gaoliming <gaoliming@byosoft.com.cn>
> Sent: Wednesday, May 19, 2021 8:59 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; 'Ard Biesheuvel' <ardb@kernel.org>;
> 'edk2-devel-groups-io' <devel@edk2.groups.io>
> Cc: sergei@posteo.net; Yao, Jiewen <jiewen.yao@intel.com>; Lu, XiaoyuX
> <xiaoyux.lu@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>
> Subject: 回复: [edk2-devel] [PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix possible
> uninitialized use
> 
> Jian:
>   These three patches are separate. They don't impact others. So, I think we can
> merge single one.
> 
> Thanks
> Liming
> > -----邮件原件-----
> > 发件人: Wang, Jian J <jian.j.wang@intel.com>
> > 发送时间: 2021年5月18日 15:36
> > 收件人: Ard Biesheuvel <ardb@kernel.org>; edk2-devel-groups-io
> > <devel@edk2.groups.io>; Liming Gao (Byosoft address)
> > <gaoliming@byosoft.com.cn>
> > 抄送: sergei@posteo.net; Yao, Jiewen <jiewen.yao@intel.com>; Lu, XiaoyuX
> > <xiaoyux.lu@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>
> > 主题: RE: [edk2-devel] [PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix possible
> > uninitialized use
> >
> > Ard,
> >
> > Patch 1&2 haven't got r-b. I'm not sure we can merge patch 3 separately.
> >
> > Regards,
> > Jian
> >
> > > -----Original Message-----
> > > From: Ard Biesheuvel <ardb@kernel.org>
> > > Sent: Tuesday, May 18, 2021 3:27 PM
> > > To: edk2-devel-groups-io <devel@edk2.groups.io>; Liming Gao (Byosoft
> > address)
> > > <gaoliming@byosoft.com.cn>
> > > Cc: sergei@posteo.net; Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> > > <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>; Jiang,
> > Guomin
> > > <guomin.jiang@intel.com>
> > > Subject: Re: [edk2-devel] [PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix
> > possible
> > > uninitialized use
> > >
> > > Please merge this fix asap. Our CI is broken because of it, and we are
> > > in the soft freeze so we need the CI up and running to catch potential
> > > issues before the release.
> > >
> > > Thanks,
> > > Ard.
> > >
> > > On Tue, 18 May 2021 at 02:59, gaoliming <gaoliming@byosoft.com.cn>
> > wrote:
> > > >
> > > > Sergei:
> > > >   Yes. GCC49 is LTO disable GCC tool chain. GCC5 is LTO enable tool
> > chain.
> > > > They both work on the different GCC version, such as gcc5, gcc6..
> > > >
> > > >   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90844 mentions
> > > > -ffat-lto-objects option that can trig the warning with LTO option. Do you
> > > > try it?
> > > >
> > > >   If this option works, we can update GCC5 tool chain definition in
> > > > tools_def.txt, then this issue can be detected in CI GCC5 build.
> > > >
> > > > Thanks
> > > > Liming
> > > > > -----邮件原件-----
> > > > > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Sergei
> > > > > Dmitrouk
> > > > > 发送时间: 2021年5月15日 21:01
> > > > > 收件人: devel@edk2.groups.io; jiewen.yao@intel.com
> > > > > 抄送: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
> > > > > <xiaoyux.lu@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>
> > > > > 主题: Re: [edk2-devel] [PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix
> > possible
> > > > > uninitialized use
> > > > >
> > > > > Hello Jiewen,
> > > > >
> > > > > I get the error only for GCC49 and not for GCC5 toolchain.  CI uses
> > GCC5.
> > > > >
> > > > > So I compared build commands and this seems to depend on LTO.
> > Adding
> > > > > `-flto`
> > > > > impedes compiler's ability to detect such simple issues.
> > > > >
> > > > > I've found relevant bug report, there is even fix suggestion from last
> > > > month:
> > > > >
> > > > >     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90844
> > > > >
> > > > > Regards,
> > > > > Sergei
> > > > >
> > > > > On Sat, May 15, 2021 at 12:30:44AM +0000, Yao, Jiewen wrote:
> > > > > > Hi Sergei
> > > > > > Thank you very much for the fix.
> > > > > > Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>
> > > > > >
> > > > > > I am a little surprised why it is not caught before. It is an obvious
> > > > logic issue.
> > > > > >
> > > > > > Do you think we can do anything on CI, to catch it during pre-check-in
> > > > in the
> > > > > future?
> > > > > > I just feel it is burden to make it post-check-in fix.
> > > > > >
> > > > > >
> > > > > > Thank you
> > > > > > Yao Jiewen
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Sergei Dmitrouk <sergei@posteo.net>
> > > > > > > Sent: Friday, May 14, 2021 8:17 PM
> > > > > > > To: devel@edk2.groups.io
> > > > > > > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> > > > > <jian.j.wang@intel.com>;
> > > > > > > Lu, XiaoyuX <xiaoyux.lu@intel.com>; Jiang, Guomin
> > > > > <guomin.jiang@intel.com>
> > > > > > > Subject: [PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix possible
> > > > uninitialized
> > > > > use
> > > > > > >
> > > > > > > `Result` can be used uninitialized in both functions after following
> > > > > > > either first or second `goto` statement.
> > > > > > >
> > > > > > > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > > > > > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > > > > > Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
> > > > > > > Cc: Guomin Jiang <guomin.jiang@intel.com>
> > > > > > > Signed-off-by: Sergei Dmitrouk <sergei@posteo.net>
> > > > > > > ---
> > > > > > >  CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c     | 1 +
> > > > > > >  CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c | 1 +
> > > > > > >  2 files changed, 2 insertions(+)
> > > > > > >
> > > > > > > diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> > > > > > > b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> > > > > > > index 4009d37d5f91..0b2960f06c4c 100644
> > > > > > > --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> > > > > > > +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> > > > > > > @@ -82,6 +82,7 @@ RsaPssVerify (
> > > > > > >    EVP_PKEY_CTX *KeyCtx;
> > > > > > >    CONST EVP_MD  *HashAlg;
> > > > > > >
> > > > > > > +  Result = FALSE;
> > > > > > >    EvpRsaKey = NULL;
> > > > > > >    EvpVerifyCtx = NULL;
> > > > > > >    KeyCtx = NULL;
> > > > > > > diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> > > > > > > b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> > > > > > > index b66b6f7296ad..ece765f9ae0a 100644
> > > > > > > --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> > > > > > > +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> > > > > > > @@ -97,6 +97,7 @@ RsaPssSign (
> > > > > > >    EVP_PKEY_CTX          *KeyCtx;
> > > > > > >    CONST EVP_MD          *HashAlg;
> > > > > > >
> > > > > > > +  Result = FALSE;
> > > > > > >    EvpRsaKey = NULL;
> > > > > > >    EvpVerifyCtx = NULL;
> > > > > > >    KeyCtx = NULL;
> > > > > > > --
> > > > > > > 2.17.6
> > > > >
> > > > >
> > > > >
> > > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > 
> > > >
> > > >
> 

回复: 回复: [edk2-devel] [PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix possible uninitialized use

[gaoliming]

Sergei:
  Thanks for your confirmation. I will send the patch to update GCC5 tool chain with this option. 

Thanks
Liming
> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Sergei
> Dmitrouk
> 发送时间: 2021年5月19日 0:03
> 收件人: devel@edk2.groups.io; gaoliming@byosoft.com.cn
> 抄送: jiewen.yao@intel.com; 'Wang, Jian J' <jian.j.wang@intel.com>; 'Lu,
> XiaoyuX' <xiaoyux.lu@intel.com>; 'Jiang, Guomin' <guomin.jiang@intel.com>
> 主题: Re: 回复: [edk2-devel] [PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix
> possible uninitialized use
> 
> Yes, adding `-ffat-lto-objects` makes the warning appear even with GCC 5.5.0.
> 
> Regards,
> Sergei
> 
> On Tue, May 18, 2021 at 08:59:05AM +0800, gaoliming wrote:
> > Sergei:
> >   Yes. GCC49 is LTO disable GCC tool chain. GCC5 is LTO enable tool chain.
> > They both work on the different GCC version, such as gcc5, gcc6..
> >
> >   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90844 mentions
> > -ffat-lto-objects option that can trig the warning with LTO option. Do you
> > try it?
> >
> >   If this option works, we can update GCC5 tool chain definition in
> > tools_def.txt, then this issue can be detected in CI GCC5 build.
> >
> > Thanks
> > Liming
> > > -----邮件原件-----
> > > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Sergei
> > > Dmitrouk
> > > 发送时间: 2021年5月15日 21:01
> > > 收件人: devel@edk2.groups.io; jiewen.yao@intel.com
> > > 抄送: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
> > > <xiaoyux.lu@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>
> > > 主题: Re: [edk2-devel] [PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix
> possible
> > > uninitialized use
> > >
> > > Hello Jiewen,
> > >
> > > I get the error only for GCC49 and not for GCC5 toolchain.  CI uses GCC5.
> > >
> > > So I compared build commands and this seems to depend on LTO.  Adding
> > > `-flto`
> > > impedes compiler's ability to detect such simple issues.
> > >
> > > I've found relevant bug report, there is even fix suggestion from last
> > month:
> > >
> > >     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90844
> > >
> > > Regards,
> > > Sergei
> > >
> > > On Sat, May 15, 2021 at 12:30:44AM +0000, Yao, Jiewen wrote:
> > > > Hi Sergei
> > > > Thank you very much for the fix.
> > > > Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>
> > > >
> > > > I am a little surprised why it is not caught before. It is an obvious
> > logic issue.
> > > >
> > > > Do you think we can do anything on CI, to catch it during pre-check-in
> > in the
> > > future?
> > > > I just feel it is burden to make it post-check-in fix.
> > > >
> > > >
> > > > Thank you
> > > > Yao Jiewen
> > > >
> > > > > -----Original Message-----
> > > > > From: Sergei Dmitrouk <sergei@posteo.net>
> > > > > Sent: Friday, May 14, 2021 8:17 PM
> > > > > To: devel@edk2.groups.io
> > > > > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> > > <jian.j.wang@intel.com>;
> > > > > Lu, XiaoyuX <xiaoyux.lu@intel.com>; Jiang, Guomin
> > > <guomin.jiang@intel.com>
> > > > > Subject: [PATCH v1 3/3] CryptoPkg/BaseCryptLib: Fix possible
> > uninitialized
> > > use
> > > > >
> > > > > `Result` can be used uninitialized in both functions after following
> > > > > either first or second `goto` statement.
> > > > >
> > > > > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > > > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > > > Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
> > > > > Cc: Guomin Jiang <guomin.jiang@intel.com>
> > > > > Signed-off-by: Sergei Dmitrouk <sergei@posteo.net>
> > > > > ---
> > > > >  CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c     | 1 +
> > > > >  CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c | 1 +
> > > > >  2 files changed, 2 insertions(+)
> > > > >
> > > > > diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> > > > > b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> > > > > index 4009d37d5f91..0b2960f06c4c 100644
> > > > > --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> > > > > +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPss.c
> > > > > @@ -82,6 +82,7 @@ RsaPssVerify (
> > > > >    EVP_PKEY_CTX *KeyCtx;
> > > > >    CONST EVP_MD  *HashAlg;
> > > > >
> > > > > +  Result = FALSE;
> > > > >    EvpRsaKey = NULL;
> > > > >    EvpVerifyCtx = NULL;
> > > > >    KeyCtx = NULL;
> > > > > diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> > > > > b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> > > > > index b66b6f7296ad..ece765f9ae0a 100644
> > > > > --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> > > > > +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptRsaPssSign.c
> > > > > @@ -97,6 +97,7 @@ RsaPssSign (
> > > > >    EVP_PKEY_CTX          *KeyCtx;
> > > > >    CONST EVP_MD          *HashAlg;
> > > > >
> > > > > +  Result = FALSE;
> > > > >    EvpRsaKey = NULL;
> > > > >    EvpVerifyCtx = NULL;
> > > > >    KeyCtx = NULL;
> > > > > --
> > > > > 2.17.6
> > >
> > >
> > >
> > >
> >
> >
> >
> >
> >
> >
> >
> >
> 
> 
> 
>