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 Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 61E28C64EC4 for ; Fri, 3 Mar 2023 19:17:56 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 67DBB85D1C; Fri, 3 Mar 2023 20:17:54 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=linux.ibm.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=ibm.com header.i=@ibm.com header.b="tZJiGUri"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 533EE85D7B; Fri, 3 Mar 2023 20:17:52 +0100 (CET) Received: from mx0b-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com [148.163.158.5]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 7940585CD0 for ; Fri, 3 Mar 2023 20:17:49 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=linux.ibm.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=eajames@linux.ibm.com Received: from pps.filterd (m0098421.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 323IuvQq026090; Fri, 3 Mar 2023 19:17:43 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ibm.com; h=message-id : date : mime-version : subject : to : cc : references : from : in-reply-to : content-type : content-transfer-encoding; s=pp1; bh=p+J2oXRDb76baxXR3MNP+NrYXrYfbOqKwQ/OrQnxmzY=; b=tZJiGUri07v51WSxHV0GLd1hPFzIRtE9egEECLjyBqJi6C7VfYF38WPOsQVkPFjIrNjv UBwoy1dNJjwTtTgbFMSLhN198tHJNZUY0RUDadkspWQuYM3mgRV7MEmKWcdLseRDWokb Efc7M7vjCVrISG3NBW2SLJ2e9KaPLMe0+rCs1CrJ16cDhIy/ICMDuhK7lrPz3TqkMTt+ mOppgVEP+IhTVHBrDqPdxbgqiFE2oViMhSZYqwHyWw8l3HYkc8eDPc9QecYZso4pqLdG UGSxMiLAZg/sbnNJ8edoY+0q/Q3gy8yroGwNiyf67Uv/wjXNbwOGLlOI2ozHhlq5ba+C xg== Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3p3pmdrg4h-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 03 Mar 2023 19:17:43 +0000 Received: from m0098421.ppops.net (m0098421.ppops.net [127.0.0.1]) by pps.reinject (8.17.1.5/8.17.1.5) with ESMTP id 323Iw42g028758; Fri, 3 Mar 2023 19:17:42 GMT Received: from ppma01wdc.us.ibm.com (fd.55.37a9.ip4.static.sl-reverse.com [169.55.85.253]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3p3pmdrg49-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 03 Mar 2023 19:17:42 +0000 Received: from pps.filterd (ppma01wdc.us.ibm.com [127.0.0.1]) by ppma01wdc.us.ibm.com (8.17.1.19/8.17.1.19) with ESMTP id 323HTVto005964; Fri, 3 Mar 2023 19:17:42 GMT Received: from smtprelay04.wdc07v.mail.ibm.com ([9.208.129.114]) by ppma01wdc.us.ibm.com (PPS) with ESMTPS id 3nybcgmkmn-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 03 Mar 2023 19:17:42 +0000 Received: from smtpav03.wdc07v.mail.ibm.com (smtpav03.wdc07v.mail.ibm.com [10.39.53.230]) by smtprelay04.wdc07v.mail.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 323JHfiE36373236 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Fri, 3 Mar 2023 19:17:41 GMT Received: from smtpav03.wdc07v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 0CD7858054; Fri, 3 Mar 2023 19:17:41 +0000 (GMT) Received: from smtpav03.wdc07v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 4978E5805D; Fri, 3 Mar 2023 19:17:40 +0000 (GMT) Received: from [9.77.137.234] (unknown [9.77.137.234]) by smtpav03.wdc07v.mail.ibm.com (Postfix) with ESMTP; Fri, 3 Mar 2023 19:17:40 +0000 (GMT) Message-ID: <3fe17233-b69f-0bc7-bd8c-6290de555fe2@linux.ibm.com> Date: Fri, 3 Mar 2023 13:17:39 -0600 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.6.0 Subject: Re: [PATCH v7 3/6] tpm: Support boot measurements To: Ilias Apalodimas Cc: u-boot@lists.denx.de, sjg@chromium.org, xypron.glpk@gmx.de, joel@jms.id.au References: <20230301225056.1402722-1-eajames@linux.ibm.com> <20230301225056.1402722-4-eajames@linux.ibm.com> Content-Language: en-US From: Eddie James In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 X-Proofpoint-GUID: cKjc8EIYiWVlOmjLWXH1DeMFgVkZK33H X-Proofpoint-ORIG-GUID: 31cS-4G97yNK0jDAi9_ZSxYR0UAF-lJ9 X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.219,Aquarius:18.0.942,Hydra:6.0.573,FMLib:17.11.170.22 definitions=2023-03-03_04,2023-03-03_01,2023-02-09_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 impostorscore=0 lowpriorityscore=0 malwarescore=0 adultscore=0 spamscore=0 suspectscore=0 phishscore=0 priorityscore=1501 bulkscore=0 mlxscore=0 clxscore=1015 mlxlogscore=999 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2212070000 definitions=main-2303030159 X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean On 3/2/23 14:22, Ilias Apalodimas wrote: > Hi Eddie, > > I found the issue. I still think we could squeeze things even more in our > abstraction. Specifically the measure_event() tcg2_agile_log_append() > contain some efi specific bits and I am trying to figure out if we can make > those more generic. However, that's not a show stopper for me. > > [...] > >> +int tcg2_init_log(struct udevice *dev, struct tcg2_event_log *elog); > We have tcg2_init_log() and tcg2_log_init(). This is a bit confusing when > reading the code. > Since tcg2_log_init() is actually initializing the EventLog can we do > tcg2_init_log -> tcg2_prepare_log_buf or something along those lines? Sure, sounds good. > >> + >> +/** >> + * Begin measurements. >> + * >> + * @dev TPM device > [...] > >> +static int tcg2_log_parse(struct udevice *dev, struct tcg2_event_log *elog) >> +{ >> + struct tpml_digest_values digest_list; >> + struct tcg_efi_spec_id_event *event; >> + struct tcg_pcr_event *log; >> + u32 calc_size; >> + u32 active; >> + u32 count; >> + u32 evsz; >> + u32 mask; >> + u16 algo; >> + u16 len; >> + int rc; >> + u32 i; >> + u16 j; >> + >> + if (elog->log_size <= offsetof(struct tcg_pcr_event, event)) >> + return 0; >> + >> + log = (struct tcg_pcr_event *)elog->log; >> + if (get_unaligned_le32(&log->pcr_index) != 0 || >> + get_unaligned_le32(&log->event_type) != EV_NO_ACTION) >> + return 0; >> + >> + for (i = 0; i < sizeof(log->digest); i++) { >> + if (log->digest[i]) >> + return 0; >> + } >> + >> + evsz = get_unaligned_le32(&log->event_size); >> + if (evsz < offsetof(struct tcg_efi_spec_id_event, digest_sizes) || >> + evsz + offsetof(struct tcg_pcr_event, event) > elog->log_size) >> + return 0; >> + >> + event = (struct tcg_efi_spec_id_event *)log->event; >> + if (memcmp(event->signature, TCG_EFI_SPEC_ID_EVENT_SIGNATURE_03, >> + sizeof(TCG_EFI_SPEC_ID_EVENT_SIGNATURE_03))) >> + return 0; >> + >> + if (event->spec_version_minor != TCG_EFI_SPEC_ID_EVENT_SPEC_VERSION_MINOR_TPM2 || >> + event->spec_version_major != TCG_EFI_SPEC_ID_EVENT_SPEC_VERSION_MAJOR_TPM2) >> + return 0; >> + >> + count = get_unaligned_le32(&event->number_of_algorithms); >> + if (count > ARRAY_SIZE(tpm2_supported_algorithms)) >> + return 0; >> + >> + calc_size = offsetof(struct tcg_efi_spec_id_event, digest_sizes) + >> + (sizeof(struct tcg_efi_spec_id_event_algorithm_size) * count) + >> + 1; >> + if (evsz != calc_size) >> + return 0; >> + >> + rc = tcg2_get_active_pcr_banks(dev, &active); >> + if (rc) >> + return rc; > There was a check here in the previous version which I can't find. The > previous stage bootloader is creating an EventLog. Since it can't control > the TPM the pcr banks that end up in the EventLog are defined at compile > time. This isn't ideal, but we have 2 options here: > 1. Check the hardware active PCR banks and report an error if there's a > mismatch (which is what the older version did) > 2. Add the missing function of re-configuring the active banks to > whatever the previous bootloader tells us. > > Obviously (2) is a better option, but I am fine if we just report an error > for now. Yes I found it, and I will add that. > > [...] > >> + *((u8 *)ev + (event_size - 1)) = 0; >> + elog->log_position = log_size; >> + >> + return 0; >> +} >> + >> +static int tcg2_log_find_end(struct tcg2_event_log *elog, struct udevice *dev, > Can we find a better name for this? This basically replays an eventlog we > inherited from a previous stage boot loader into the TPM. > So something like tcg2_replay_eventlog()? Sure. > >> + struct tpml_digest_values *digest_list) >> +{ >> + const u32 offset = offsetof(struct tcg_pcr_event2, digests) + >> + offsetof(struct tpml_digest_values, digests); >> + u32 event_size; >> + u32 count; >> + u16 algo; >> + u32 pcr; >> + u32 pos; >> + u16 len; >> + u8 *log; >> + int rc; >> + u32 i; >> + >> + while (elog->log_position + offset < elog->log_size) { >> + log = elog->log + elog->log_position; >> + >> + pos = offsetof(struct tcg_pcr_event2, pcr_index); >> + pcr = get_unaligned_le32(log + pos); >> + pos = offsetof(struct tcg_pcr_event2, event_type); >> + if (!get_unaligned_le32(log + pos)) >> + return 0; > isn't this an actual error ? Good point, and all below too. I will return errors there. > >> + >> + pos = offsetof(struct tcg_pcr_event2, digests) + >> + offsetof(struct tpml_digest_values, count); >> + count = get_unaligned_le32(log + pos); >> + if (count > ARRAY_SIZE(tpm2_supported_algorithms) || >> + (digest_list->count && digest_list->count != count)) >> + return 0; > ditto > >> + >> + pos = offsetof(struct tcg_pcr_event2, digests) + >> + offsetof(struct tpml_digest_values, digests); >> + for (i = 0; i < count; ++i) { >> + pos += offsetof(struct tpmt_ha, hash_alg); >> + if (elog->log_position + pos + sizeof(u16) >= >> + elog->log_size) >> + return 0; > ditto > >> + >> + algo = get_unaligned_le16(log + pos); >> + pos += offsetof(struct tpmt_ha, digest); >> + switch (algo) { >> + case TPM2_ALG_SHA1: >> + case TPM2_ALG_SHA256: >> + case TPM2_ALG_SHA384: >> + case TPM2_ALG_SHA512: >> + len = tpm2_algorithm_to_len(algo); >> + break; >> + default: >> + return 0; > I think we should return errors in this case. Otherwise we'll end up with > an incomplete view of either the TPM or the extended PCRs > >> + } >> + >> + if (digest_list->count) { >> + if (algo != digest_list->digests[i].hash_alg || >> + elog->log_position + pos + len >= >> + elog->log_size) >> + return 0; >> + >> + memcpy(digest_list->digests[i].digest.sha512, >> + log + pos, len); >> + } >> + >> + pos += len; >> + } >> + >> + if (elog->log_position + pos + sizeof(u32) >= elog->log_size) >> + return 0; >> + >> + event_size = get_unaligned_le32(log + pos); >> + pos += event_size + sizeof(u32); >> + if (elog->log_position + pos >= elog->log_size) > This is off by one and as a result you skip replaying the last > event. This should be > if (elog->log_position + pos > elog->log_size) Nice catch, thank you! I'll fix it. Thanks for your review! v8 coming soon. Eddie > .... > > [...] > > Cheers > /Ilias