SecurityPkg/Library/Tpm2CommandLib/Tpm2Help.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Add 'Index < HASH_COUNT' to ensure things out of boundary
of digests[] can not be visited.
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
---
SecurityPkg/Library/Tpm2CommandLib/Tpm2Help.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/SecurityPkg/Library/Tpm2CommandLib/Tpm2Help.c b/SecurityPkg/Library/Tpm2CommandLib/Tpm2Help.c
index 36c240d1221c..a7d4e3ab5373 100644
--- a/SecurityPkg/Library/Tpm2CommandLib/Tpm2Help.c
+++ b/SecurityPkg/Library/Tpm2CommandLib/Tpm2Help.c
@@ -299,7 +299,7 @@ GetDigestListSize (
UINT32 TotalSize;
TotalSize = sizeof(DigestList->count);
- for (Index = 0; Index < DigestList->count; Index++) {
+ for (Index = 0; Index < DigestList->count, Index < HASH_COUNT; Index++) {
DigestSize = GetHashSizeFromAlgo (DigestList->digests[Index].hashAlg);
TotalSize += sizeof(DigestList->digests[Index].hashAlg) + DigestSize;
}
--
2.18.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#51807): https://edk2.groups.io/g/devel/message/51807
Mute This Topic: https://groups.io/mt/67403438/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Hi May I know where is the data from? Trusted region or non-trusted region? I am thinking if we need use ASSERT to avoid user mistake. But want to check the API input assumption at first... > -----Original Message----- > From: Zhang, Shenglei <shenglei.zhang@intel.com> > Sent: Friday, December 6, 2019 9:50 AM > To: devel@edk2.groups.io > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; > Zhang, Chao B <chao.b.zhang@intel.com> > Subject: [PATCH] SecurityPkg/Tpm2Help.c: Add boundary check for array > > Add 'Index < HASH_COUNT' to ensure things out of boundary > of digests[] can not be visited. > > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Jian J Wang <jian.j.wang@intel.com> > Cc: Chao Zhang <chao.b.zhang@intel.com> > Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com> > --- > SecurityPkg/Library/Tpm2CommandLib/Tpm2Help.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/SecurityPkg/Library/Tpm2CommandLib/Tpm2Help.c > b/SecurityPkg/Library/Tpm2CommandLib/Tpm2Help.c > index 36c240d1221c..a7d4e3ab5373 100644 > --- a/SecurityPkg/Library/Tpm2CommandLib/Tpm2Help.c > +++ b/SecurityPkg/Library/Tpm2CommandLib/Tpm2Help.c > @@ -299,7 +299,7 @@ GetDigestListSize ( > UINT32 TotalSize; > > TotalSize = sizeof(DigestList->count); > - for (Index = 0; Index < DigestList->count; Index++) { > + for (Index = 0; Index < DigestList->count, Index < HASH_COUNT; Index++) { > DigestSize = GetHashSizeFromAlgo (DigestList->digests[Index].hashAlg); > TotalSize += sizeof(DigestList->digests[Index].hashAlg) + DigestSize; > } > -- > 2.18.0.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#51808): https://edk2.groups.io/g/devel/message/51808 Mute This Topic: https://groups.io/mt/67403438/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
> -----Original Message----- > From: Yao, Jiewen > Sent: Friday, December 6, 2019 10:04 AM > To: Zhang, Shenglei <shenglei.zhang@intel.com>; devel@edk2.groups.io > Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B > <chao.b.zhang@intel.com> > Subject: RE: [PATCH] SecurityPkg/Tpm2Help.c: Add boundary check for array > > Hi > May I know where is the data from? Trusted region or non-trusted region? > > I am thinking if we need use ASSERT to avoid user mistake. > But want to check the API input assumption at first... Hi Jiewen, I don't think DigestList->count can be trusted. We can add Index < HASH_COUNT into the for(...) statement. Thanks, Shenglei > > > > > -----Original Message----- > > From: Zhang, Shenglei <shenglei.zhang@intel.com> > > Sent: Friday, December 6, 2019 9:50 AM > > To: devel@edk2.groups.io > > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J > <jian.j.wang@intel.com>; > > Zhang, Chao B <chao.b.zhang@intel.com> > > Subject: [PATCH] SecurityPkg/Tpm2Help.c: Add boundary check for array > > > > Add 'Index < HASH_COUNT' to ensure things out of boundary > > of digests[] can not be visited. > > > > Cc: Jiewen Yao <jiewen.yao@intel.com> > > Cc: Jian J Wang <jian.j.wang@intel.com> > > Cc: Chao Zhang <chao.b.zhang@intel.com> > > Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com> > > --- > > SecurityPkg/Library/Tpm2CommandLib/Tpm2Help.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/SecurityPkg/Library/Tpm2CommandLib/Tpm2Help.c > > b/SecurityPkg/Library/Tpm2CommandLib/Tpm2Help.c > > index 36c240d1221c..a7d4e3ab5373 100644 > > --- a/SecurityPkg/Library/Tpm2CommandLib/Tpm2Help.c > > +++ b/SecurityPkg/Library/Tpm2CommandLib/Tpm2Help.c > > @@ -299,7 +299,7 @@ GetDigestListSize ( > > UINT32 TotalSize; > > > > TotalSize = sizeof(DigestList->count); > > - for (Index = 0; Index < DigestList->count; Index++) { > > + for (Index = 0; Index < DigestList->count, Index < HASH_COUNT; Index++) > { > > DigestSize = GetHashSizeFromAlgo (DigestList->digests[Index].hashAlg); > > TotalSize += sizeof(DigestList->digests[Index].hashAlg) + DigestSize; > > } > > -- > > 2.18.0.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#52240): https://edk2.groups.io/g/devel/message/52240 Mute This Topic: https://groups.io/mt/67403438/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 12/06/19 02:49, Zhang, Shenglei wrote: > Add 'Index < HASH_COUNT' to ensure things out of boundary > of digests[] can not be visited. > > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Jian J Wang <jian.j.wang@intel.com> > Cc: Chao Zhang <chao.b.zhang@intel.com> > Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com> > --- > SecurityPkg/Library/Tpm2CommandLib/Tpm2Help.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/SecurityPkg/Library/Tpm2CommandLib/Tpm2Help.c b/SecurityPkg/Library/Tpm2CommandLib/Tpm2Help.c > index 36c240d1221c..a7d4e3ab5373 100644 > --- a/SecurityPkg/Library/Tpm2CommandLib/Tpm2Help.c > +++ b/SecurityPkg/Library/Tpm2CommandLib/Tpm2Help.c > @@ -299,7 +299,7 @@ GetDigestListSize ( > UINT32 TotalSize; > > TotalSize = sizeof(DigestList->count); > - for (Index = 0; Index < DigestList->count; Index++) { > + for (Index = 0; Index < DigestList->count, Index < HASH_COUNT; Index++) { > DigestSize = GetHashSizeFromAlgo (DigestList->digests[Index].hashAlg); > TotalSize += sizeof(DigestList->digests[Index].hashAlg) + DigestSize; > } > Nacked-by: Laszlo Ersek <lersek@redhat.com> The comma operator is either functionally wrong in this context, or stylistically wrong. From the C standard: The left operand of a comma operator is evaluated as a void expression; there is a sequence point after its evaluation. Then the right operand is evaluated; the result has its type and value. [...] In case we *only* need to check (Index < HASH_COUNT), then the patch is stylistically incorrect: the (Index < DigestList->count) condition should simply be removed. In case we need to check *both* conditions, then the patch is functionally wrong: we should either use the logical AND (&&) operator, instead of the comma: Index < DigestList->count && Index < HASH_COUNT or invoke the MIN() function-like macro: Index < MIN ((UINTN)DigestList->count, (UINTN)HASH_COUNT) Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#51824): https://edk2.groups.io/g/devel/message/51824 Mute This Topic: https://groups.io/mt/67403438/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
> -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > Laszlo Ersek > Sent: Friday, December 6, 2019 5:27 PM > To: devel@edk2.groups.io; Zhang, Shenglei <shenglei.zhang@intel.com> > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J > <jian.j.wang@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com> > Subject: Re: [edk2-devel] [PATCH] SecurityPkg/Tpm2Help.c: Add boundary > check for array > > On 12/06/19 02:49, Zhang, Shenglei wrote: > > Add 'Index < HASH_COUNT' to ensure things out of boundary > > of digests[] can not be visited. > > > > Cc: Jiewen Yao <jiewen.yao@intel.com> > > Cc: Jian J Wang <jian.j.wang@intel.com> > > Cc: Chao Zhang <chao.b.zhang@intel.com> > > Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com> > > --- > > SecurityPkg/Library/Tpm2CommandLib/Tpm2Help.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/SecurityPkg/Library/Tpm2CommandLib/Tpm2Help.c > b/SecurityPkg/Library/Tpm2CommandLib/Tpm2Help.c > > index 36c240d1221c..a7d4e3ab5373 100644 > > --- a/SecurityPkg/Library/Tpm2CommandLib/Tpm2Help.c > > +++ b/SecurityPkg/Library/Tpm2CommandLib/Tpm2Help.c > > @@ -299,7 +299,7 @@ GetDigestListSize ( > > UINT32 TotalSize; > > > > TotalSize = sizeof(DigestList->count); > > - for (Index = 0; Index < DigestList->count; Index++) { > > + for (Index = 0; Index < DigestList->count, Index < HASH_COUNT; Index++) > { > > DigestSize = GetHashSizeFromAlgo (DigestList->digests[Index].hashAlg); > > TotalSize += sizeof(DigestList->digests[Index].hashAlg) + DigestSize; > > } > > > > Nacked-by: Laszlo Ersek <lersek@redhat.com> > > The comma operator is either functionally wrong in this context, or > stylistically wrong. From the C standard: > > The left operand of a comma operator is evaluated as a void > expression; there is a sequence point after its evaluation. Then the > right operand is evaluated; the result has its type and value. [...] > > In case we *only* need to check (Index < HASH_COUNT), then the patch is > stylistically incorrect: the (Index < DigestList->count) condition > should simply be removed. > > In case we need to check *both* conditions, then the patch is > functionally wrong: we should either use the logical AND (&&) operator, > instead of the comma: > > Index < DigestList->count && Index < HASH_COUNT > Hi Laszlo, You are right. I'll change the statement to include both conditions. Thanks, Shenglei > or invoke the MIN() function-like macro: > > Index < MIN ((UINTN)DigestList->count, (UINTN)HASH_COUNT) > > Thanks > Laszlo > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#52241): https://edk2.groups.io/g/devel/message/52241 Mute This Topic: https://groups.io/mt/67403438/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.