From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 58EE3ECDE4B for ; Thu, 8 Nov 2018 13:54:13 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 0CEF52133D for ; Thu, 8 Nov 2018 13:54:12 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0CEF52133D Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.ibm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727259AbeKHX3q (ORCPT ); Thu, 8 Nov 2018 18:29:46 -0500 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:58268 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726724AbeKHX3p (ORCPT ); Thu, 8 Nov 2018 18:29:45 -0500 Received: from pps.filterd (m0098410.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.22/8.16.0.22) with SMTP id wA8DoAnn146819 for ; Thu, 8 Nov 2018 08:54:09 -0500 Received: from e36.co.us.ibm.com (e36.co.us.ibm.com [32.97.110.154]) by mx0a-001b2d01.pphosted.com with ESMTP id 2nmks2ypyd-1 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=NOT) for ; Thu, 08 Nov 2018 08:54:09 -0500 Received: from localhost by e36.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 8 Nov 2018 13:54:08 -0000 Received: from b03cxnp08028.gho.boulder.ibm.com (9.17.130.20) by e36.co.us.ibm.com (192.168.1.136) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; (version=TLSv1/SSLv3 cipher=AES256-GCM-SHA384 bits=256/256) Thu, 8 Nov 2018 13:54:04 -0000 Received: from b03ledav002.gho.boulder.ibm.com (b03ledav002.gho.boulder.ibm.com [9.17.130.233]) by b03cxnp08028.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id wA8Ds3Hw17104994 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Thu, 8 Nov 2018 13:54:03 GMT Received: from b03ledav002.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 4D761136053; Thu, 8 Nov 2018 13:54:03 +0000 (GMT) Received: from b03ledav002.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 87BD513604F; Thu, 8 Nov 2018 13:53:59 +0000 (GMT) Received: from [9.77.212.72] (unknown [9.77.212.72]) by b03ledav002.gho.boulder.ibm.com (Postfix) with ESMTP; Thu, 8 Nov 2018 13:53:59 +0000 (GMT) Subject: Re: [PATCH v4 1/6] tpm: dynamically allocate active_banks array To: Roberto Sassu , jarkko.sakkinen@linux.intel.com, zohar@linux.ibm.com Cc: linux-integrity@vger.kernel.org, linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org, silviu.vlasceanu@huawei.com, Ken Goldman , Kenneth Goldman References: <20181106150159.1136-1-roberto.sassu@huawei.com> <20181106150159.1136-2-roberto.sassu@huawei.com> <98482eee-6e91-1666-1ce2-cfa94a33efc2@linux.ibm.com> From: Nayna Jain Date: Thu, 8 Nov 2018 19:20:51 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US X-TM-AS-GCONF: 00 x-cbid: 18110813-0020-0000-0000-00000E871F4D X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00010008; HX=3.00000242; KW=3.00000007; PH=3.00000004; SC=3.00000268; SDB=6.01114466; UDB=6.00577800; IPR=6.00894565; MB=3.00024075; MTD=3.00000008; XFM=3.00000015; UTC=2018-11-08 13:54:06 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 18110813-0021-0000-0000-000063A67B30 Message-Id: <086944ab-dd56-5522-af26-e9bb545556fd@linux.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2018-11-08_07:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 malwarescore=0 suspectscore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1015 lowpriorityscore=0 mlxscore=0 impostorscore=0 mlxlogscore=999 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1807170000 definitions=main-1811080118 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/07/2018 03:11 PM, Roberto Sassu wrote: > On 11/7/2018 7:14 AM, Nayna Jain wrote: >> >> >> On 11/06/2018 08:31 PM, Roberto Sassu wrote: >>> This patch removes the hard-coded limit of the active_banks array size. >> >> >> The hard-coded limit in static array active_banks[] represents the >> maximum possible banks. >> A TPM might have three banks, but only one bank may be active. >> >> To confirm my understanding, is the idea for this patch is to >> dynamically identify the number of possible banks or the number of >> active banks ? > > The idea is to dynamically identify the number of active banks. > > In the TPM Commands specification (section 30.2.1), I found: > > TPM_CAP_PCRS – Returns the current allocation of PCR in a > TPML_PCR_SELECTION. > > You mentioned: > > #TPM_RC_SIZE response code when count is greater > than the possible number of banks > > but TPML_PCR_SELECTION is provided by the TPM. Based on a discussion with Ken, the count in the TPML_PCR_SELECTION returns the number of possible algorithms supported. In the example below, two possible algorithms - SHA1 and SHA256 - are returned. # /usr/local/bin/tssgetcapability -cap 5 2 PCR selections     hash TPM_ALG_SHA1     TPMS_PCR_SELECTION length 3     ff ff ff     hash TPM_ALG_SHA256     TPMS_PCR_SELECTION length 3     00 00 00 The pcr_select fields - "ff ff ff" and "00 00 00" - are bit masks for the enabled PCRs. The SHA1 bank is enabled for all PCRs (0-23), while the SHA256 bank is not enabled. The current code works, but it unnecessarily extends some banks. Instead of basing the number of active banks on the number of algorithms returned, it should be based on the pcr_select field.    - Mimi & Nayna > > Roberto > > >>> It stores in the tpm_chip structure the number of active PCR banks, >>> determined in tpm2_get_pcr_allocation(), and replaces the static array >>> with a pointer to a dynamically allocated array. >>> >>> As a consequence of the introduction of nr_active_banks, >>> tpm_pcr_extend() >>> does not check anymore if the algorithm stored in tpm_chip is equal to >>> zero. The active_banks array always contains valid algorithms. >>> >>> Fixes: 1db15344f874 ("tpm: implement TPM 2.0 capability to get active >>> PCR banks") >>> >>> Signed-off-by: Roberto Sassu >>> --- >>>   drivers/char/tpm/tpm-chip.c      |  1 + >>>   drivers/char/tpm/tpm-interface.c | 19 ++++++++++++------- >>>   drivers/char/tpm/tpm.h           |  3 ++- >>>   drivers/char/tpm/tpm2-cmd.c      | 17 ++++++++--------- >>>   4 files changed, 23 insertions(+), 17 deletions(-) >>> >>> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c >>> index 46caadca916a..2a9e8b744436 100644 >>> --- a/drivers/char/tpm/tpm-chip.c >>> +++ b/drivers/char/tpm/tpm-chip.c >>> @@ -160,6 +160,7 @@ static void tpm_dev_release(struct device *dev) >>>       kfree(chip->log.bios_event_log); >>>       kfree(chip->work_space.context_buf); >>>       kfree(chip->work_space.session_buf); >>> +    kfree(chip->active_banks); >>>       kfree(chip); >>>   } >>> >>> diff --git a/drivers/char/tpm/tpm-interface.c >>> b/drivers/char/tpm/tpm-interface.c >>> index 1a803b0cf980..ba7ca6b3e664 100644 >>> --- a/drivers/char/tpm/tpm-interface.c >>> +++ b/drivers/char/tpm/tpm-interface.c >>> @@ -1039,8 +1039,7 @@ static int tpm1_pcr_extend(struct tpm_chip >>> *chip, int pcr_idx, const u8 *hash, >>>   int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 >>> *hash) >>>   { >>>       int rc; >>> -    struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)]; >>> -    u32 count = 0; >>> +    struct tpm2_digest *digest_list; >>>       int i; >>> >>>       chip = tpm_find_get_ops(chip); >>> @@ -1048,16 +1047,22 @@ int tpm_pcr_extend(struct tpm_chip *chip, >>> int pcr_idx, const u8 *hash) >>>           return -ENODEV; >>> >>>       if (chip->flags & TPM_CHIP_FLAG_TPM2) { >>> -        memset(digest_list, 0, sizeof(digest_list)); >>> +        digest_list = kmalloc_array(chip->nr_active_banks, >>> +                        sizeof(*digest_list), GFP_KERNEL); >>> +        if (!digest_list) >>> +            return -ENOMEM; >>> >>> -        for (i = 0; i < ARRAY_SIZE(chip->active_banks) && >>> -                chip->active_banks[i] != TPM2_ALG_ERROR; i++) { >>> +        memset(digest_list, 0, >>> +               chip->nr_active_banks * sizeof(*digest_list)); >>> + >>> +        for (i = 0; i < chip->nr_active_banks; i++) { >>>               digest_list[i].alg_id = chip->active_banks[i]; >>>               memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE); >>> -            count++; >>>           } >>> >>> -        rc = tpm2_pcr_extend(chip, pcr_idx, count, digest_list); >>> +        rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_active_banks, >>> +                     digest_list); >>> +        kfree(digest_list); >>>           tpm_put_ops(chip); >>>           return rc; >>>       } >>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h >>> index f3501d05264f..98368c3a6ff7 100644 >>> --- a/drivers/char/tpm/tpm.h >>> +++ b/drivers/char/tpm/tpm.h >>> @@ -248,7 +248,8 @@ struct tpm_chip { >>>       const struct attribute_group *groups[3]; >>>       unsigned int groups_cnt; >>> >>> -    u16 active_banks[7]; >>> +    u32 nr_active_banks; >>> +    u16 *active_banks; >>>   #ifdef CONFIG_ACPI >>>       acpi_handle acpi_dev_handle; >>>       char ppi_version[TPM_PPI_VERSION_LEN + 1]; >>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c >>> index c31b490bd41d..533089cede07 100644 >>> --- a/drivers/char/tpm/tpm2-cmd.c >>> +++ b/drivers/char/tpm/tpm2-cmd.c >>> @@ -242,7 +242,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int >>> pcr_idx, u32 count, >>>       int i; >>>       int j; >>> >>> -    if (count > ARRAY_SIZE(chip->active_banks)) >>> +    if (count > chip->nr_active_banks) >>>           return -EINVAL; >>> >>>       rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND); >>> @@ -859,7 +859,6 @@ static ssize_t tpm2_get_pcr_allocation(struct >>> tpm_chip *chip) >>>       void *marker; >>>       void *end; >>>       void *pcr_select_offset; >>> -    unsigned int count; >>>       u32 sizeof_pcr_selection; >>>       u32 rsp_len; >>>       int rc; >>> @@ -878,11 +877,14 @@ static ssize_t tpm2_get_pcr_allocation(struct >>> tpm_chip *chip) >>>       if (rc) >>>           goto out; >>> >>> -    count = be32_to_cpup( >>> +    chip->nr_active_banks = be32_to_cpup( >>>           (__be32 *)&buf.data[TPM_HEADER_SIZE + 5]); >> >> >> As per my understanding, the count in the TPML_PCR_SELECTION >> represent the number of possible banks and not the number of active >> banks. >> TCG Structures Spec for TPM 2.0 - Table 102 mentions this as >> explanation of #TPM_RC_SIZE. >> >> Thanks & Regards, >>      - Nayna >> >> >>> >>> -    if (count > ARRAY_SIZE(chip->active_banks)) { >>> -        rc = -ENODEV; >>> +    chip->active_banks = kmalloc_array(chip->nr_active_banks, >>> +                       sizeof(*chip->active_banks), >>> +                       GFP_KERNEL); >>> +    if (!chip->active_banks) { >>> +        rc = -ENOMEM; >>>           goto out; >>>       } >>> >>> @@ -891,7 +893,7 @@ static ssize_t tpm2_get_pcr_allocation(struct >>> tpm_chip *chip) >>>       rsp_len = be32_to_cpup((__be32 *)&buf.data[2]); >>>       end = &buf.data[rsp_len]; >>> >>> -    for (i = 0; i < count; i++) { >>> +    for (i = 0; i < chip->nr_active_banks; i++) { >>>           pcr_select_offset = marker + >>>               offsetof(struct tpm2_pcr_selection, size_of_select); >>>           if (pcr_select_offset >= end) { >>> @@ -908,9 +910,6 @@ static ssize_t tpm2_get_pcr_allocation(struct >>> tpm_chip *chip) >>>       } >>> >>>   out: >>> -    if (i < ARRAY_SIZE(chip->active_banks)) >>> -        chip->active_banks[i] = TPM2_ALG_ERROR; >>> - >>>       tpm_buf_destroy(&buf); >>> >>>       return rc; >> >