[PATCH] SecurityPkg/AuthVariableLib: Fix ptr bad arithmetic.

[PATCH] SecurityPkg/AuthVariableLib: Fix ptr bad arithmetic.

[chenc2]

Use variable instead of sizeof(UINT8) and sizeof(UINT32) to
avoid bad arithmetic of pointer.

Cc: chenc2 <chen.a.chen@intel.com>
Cc: Wu Hao A <hao.a.wu@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Cc: Zhang Chao B <chao.b.zhang@intel.com>
---
 SecurityPkg/Library/AuthVariableLib/AuthService.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/SecurityPkg/Library/AuthVariableLib/AuthService.c b/SecurityPkg/Library/AuthVariableLib/AuthService.c
index 7188ff6008..d6387d5ea6 100644
--- a/SecurityPkg/Library/AuthVariableLib/AuthService.c
+++ b/SecurityPkg/Library/AuthVariableLib/AuthService.c
@@ -2289,6 +2289,8 @@ VerifyTimeBasedPayload (
   UINT8                            *CertsInCertDb;
   UINT32                           CertsSizeinDb;
   UINT8                            Sha256Digest[SHA256_DIGEST_SIZE];
+  UINTN                            Offset1;
+  UINTN                            Offset2;
 
   //
   // 1. TopLevelCert is the top-level issuer certificate in signature Signer Cert Chain
@@ -2559,9 +2561,11 @@ VerifyTimeBasedPayload (
         //
         // Check hash of signer cert CommonName + Top-level issuer tbsCertificate against data in CertDb
         //
+        Offset1 = sizeof (UINT8) + sizeof (UINT32);
+        Offset2 = sizeof (UINT8);
         Status = CalculatePrivAuthVarSignChainSHA256Digest(
-                   SignerCerts + sizeof(UINT8) + sizeof(UINT32),
-                   ReadUnaligned32 ((UINT32 *)(SignerCerts + sizeof(UINT8))),
+                   SignerCerts + Offset1,
+                   ReadUnaligned32 ((UINT32 *)(SignerCerts + Offset2)),
                    TopLevelCert,
                    TopLevelCertSize,
                    Sha256Digest
@@ -2596,12 +2600,14 @@ VerifyTimeBasedPayload (
       //
       // When adding a new common authenticated variable, always save Hash of cn of signer cert + tbsCertificate of Top-level issuer
       //
+      Offset1 = sizeof (UINT8) + sizeof (UINT32);
+      Offset2 = sizeof (UINT8);
       Status = InsertCertsToDb (
                  VariableName,
                  VendorGuid,
                  Attributes,
-                 SignerCerts + sizeof(UINT8) + sizeof(UINT32),
-                 ReadUnaligned32 ((UINT32 *)(SignerCerts + sizeof(UINT8))),
+                 SignerCerts + Offset1,
+                 ReadUnaligned32 ((UINT32 *)(SignerCerts + Offset2)),
                  TopLevelCert,
                  TopLevelCertSize
                  );
-- 
2.13.2.windows.1

Re: [PATCH] SecurityPkg/AuthVariableLib: Fix ptr bad arithmetic.

[Laszlo Ersek]

On 10/16/17 10:43, chenc2 wrote:
> Use variable instead of sizeof(UINT8) and sizeof(UINT32) to
> avoid bad arithmetic of pointer.
> 
> Cc: chenc2 <chen.a.chen@intel.com>
> Cc: Wu Hao A <hao.a.wu@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Cc: Zhang Chao B <chao.b.zhang@intel.com>
> ---
>  SecurityPkg/Library/AuthVariableLib/AuthService.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/SecurityPkg/Library/AuthVariableLib/AuthService.c b/SecurityPkg/Library/AuthVariableLib/AuthService.c
> index 7188ff6008..d6387d5ea6 100644
> --- a/SecurityPkg/Library/AuthVariableLib/AuthService.c
> +++ b/SecurityPkg/Library/AuthVariableLib/AuthService.c
> @@ -2289,6 +2289,8 @@ VerifyTimeBasedPayload (
>    UINT8                            *CertsInCertDb;
>    UINT32                           CertsSizeinDb;
>    UINT8                            Sha256Digest[SHA256_DIGEST_SIZE];
> +  UINTN                            Offset1;
> +  UINTN                            Offset2;
>  
>    //
>    // 1. TopLevelCert is the top-level issuer certificate in signature Signer Cert Chain
> @@ -2559,9 +2561,11 @@ VerifyTimeBasedPayload (
>          //
>          // Check hash of signer cert CommonName + Top-level issuer tbsCertificate against data in CertDb
>          //
> +        Offset1 = sizeof (UINT8) + sizeof (UINT32);
> +        Offset2 = sizeof (UINT8);
>          Status = CalculatePrivAuthVarSignChainSHA256Digest(
> -                   SignerCerts + sizeof(UINT8) + sizeof(UINT32),
> -                   ReadUnaligned32 ((UINT32 *)(SignerCerts + sizeof(UINT8))),
> +                   SignerCerts + Offset1,
> +                   ReadUnaligned32 ((UINT32 *)(SignerCerts + Offset2)),
>                     TopLevelCert,
>                     TopLevelCertSize,
>                     Sha256Digest
> @@ -2596,12 +2600,14 @@ VerifyTimeBasedPayload (
>        //
>        // When adding a new common authenticated variable, always save Hash of cn of signer cert + tbsCertificate of Top-level issuer
>        //
> +      Offset1 = sizeof (UINT8) + sizeof (UINT32);
> +      Offset2 = sizeof (UINT8);
>        Status = InsertCertsToDb (
>                   VariableName,
>                   VendorGuid,
>                   Attributes,
> -                 SignerCerts + sizeof(UINT8) + sizeof(UINT32),
> -                 ReadUnaligned32 ((UINT32 *)(SignerCerts + sizeof(UINT8))),
> +                 SignerCerts + Offset1,
> +                 ReadUnaligned32 ((UINT32 *)(SignerCerts + Offset2)),
>                   TopLevelCert,
>                   TopLevelCertSize
>                   );
> 

I don't understand how this patch makes any difference.

Thanks
Laszlo