From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.81]) by mx.groups.io with SMTP id smtpd.web09.1446.1575624440596923769 for ; Fri, 06 Dec 2019 01:27:20 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=VEaeK2WO; spf=pass (domain: redhat.com, ip: 207.211.31.81, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1575624439; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=9M79FiaDueJc/O8YzUSY7fq970+IslBqFW8yCXoKwbY=; b=VEaeK2WOUd6Fe/m/IiGO+n+PVkAFUjcjDJe1HZ6P7RzpdODZ3FgHJzlmUIVVUHAiG33xBx SFzrUZLRZRrSOuT69mH3h6m9sjgdn9welMTvMNsrYnIw3WDBPaPol3Dui/LQoj9yi+DwhC WMqI8/mmTNaWX8Kis94qmVNVBOyZSHM= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-163-jjDDh5U0PWmUQBoMbs3S-Q-1; Fri, 06 Dec 2019 04:27:16 -0500 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id E6BFA1852E21; Fri, 6 Dec 2019 09:27:14 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-235.ams2.redhat.com [10.36.116.235]) by smtp.corp.redhat.com (Postfix) with ESMTP id 94AB919C4F; Fri, 6 Dec 2019 09:27:13 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] SecurityPkg/Tpm2Help.c: Add boundary check for array To: devel@edk2.groups.io, shenglei.zhang@intel.com Cc: Jiewen Yao , Jian J Wang , Chao Zhang References: <20191206014933.36648-1-shenglei.zhang@intel.com> From: "Laszlo Ersek" Message-ID: Date: Fri, 6 Dec 2019 10:27:12 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20191206014933.36648-1-shenglei.zhang@intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-MC-Unique: jjDDh5U0PWmUQBoMbs3S-Q-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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 > Cc: Jian J Wang > Cc: Chao Zhang > Signed-off-by: Shenglei Zhang > --- > 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 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