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 8CDB3C6FA8E for ; Thu, 2 Mar 2023 20:22:40 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id F0B7D85A98; Thu, 2 Mar 2023 21:22:37 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org 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=linaro.org header.i=@linaro.org header.b="H3rLhSv3"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 2326A85B1B; Thu, 2 Mar 2023 21:22:36 +0100 (CET) Received: from mail-ed1-x533.google.com (mail-ed1-x533.google.com [IPv6:2a00:1450:4864:20::533]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 26F8F85367 for ; Thu, 2 Mar 2023 21:22:33 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=ilias.apalodimas@linaro.org Received: by mail-ed1-x533.google.com with SMTP id s26so1768889edw.11 for ; Thu, 02 Mar 2023 12:22:33 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=zSiohBhGNwDIrIH2NfgHT/udkN2y+m/TtKzwbF5dVwk=; b=H3rLhSv31qBidI7JbCzPDhK4mjj+ERTuGPw3neYXwUAxDvENh0SA7ruRM5mBuBNa4Z w+l8TrmL+w1YmvSzPOxIOER94cG8p2kJq64BGOulNBbHWeDlifGYGJDN6fQagSW8kXLy sq/6mops+yDuISxLFRxGki5ptjfBuuMoXxnAEWW+3bqYnIJaQ1s/8WSz8V4Jfg3SmUjI 9fZWSeqW3z2ylYWf7o5u1ghZOUMtxM+ZALVcSo+2/YfrKsG20K2kOUEWKSO70OMftXK2 nSrdfIyHz7TC4deIiGl5QEbo/nq73QiXuJtvHfpcBN+OZ8l7JBvuOCDI8N22CTpdUo7R rUaQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=zSiohBhGNwDIrIH2NfgHT/udkN2y+m/TtKzwbF5dVwk=; b=erMH2ck+croZF/IqoKR20s0rh+v1FJ8Jtkk713JV5rzjzBd6rDpjSkV2UNBVcI5ZWj C0BKg178n+56WMfL+F+zhXAKtx/dARmfOkzF04Eobjlh+xStVtTCEKelzLDK/9xrDP5t QG/3QqD11s6hBu40dxdFCysgxIMooj65U1C8pFCw8u3fgZMwg/fvFQsTGV/JOdPxZqq1 Qsiqz1UE1hvah1HGO3gMaM/NENfVDFbcuzm/GLHwt6cQiNwciJFNCpgVdi6g1A2RExDd BO3qEFjomxqd60M2IfscAly0qGEBtmthfhL8h4w71yXd5zz/ukGyr7ZQJE4zwJJ732Dj aUkQ== X-Gm-Message-State: AO0yUKV4e9XMjJsWKe0l6nDl8VzydaUo0q0n68T64FlHf99cHNE7sgfQ UlVOpku1z71/2riIMLD3QXDgXg== X-Google-Smtp-Source: AK7set/fbAYOOw/UmCq4ZKU/mGmwnCh8SUmwrhCicVTXihrOPC8sYF3lp3WE5R/fBLYM8O6zmRWsfA== X-Received: by 2002:aa7:d7cc:0:b0:4be:7311:1135 with SMTP id e12-20020aa7d7cc000000b004be73111135mr5457710eds.31.1677788552642; Thu, 02 Mar 2023 12:22:32 -0800 (PST) Received: from hera (ppp176092130041.access.hol.gr. [176.92.130.41]) by smtp.gmail.com with ESMTPSA id l26-20020a170906231a00b008b69aa62efcsm85914eja.62.2023.03.02.12.22.31 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 02 Mar 2023 12:22:32 -0800 (PST) Date: Thu, 2 Mar 2023 22:22:29 +0200 From: Ilias Apalodimas To: Eddie James Cc: u-boot@lists.denx.de, sjg@chromium.org, xypron.glpk@gmx.de, joel@jms.id.au Subject: Re: [PATCH v7 3/6] tpm: Support boot measurements Message-ID: References: <20230301225056.1402722-1-eajames@linux.ibm.com> <20230301225056.1402722-4-eajames@linux.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230301225056.1402722-4-eajames@linux.ibm.com> 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 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? > + > +/** > + * 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. [...] > + *((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()? > + 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 ? > + > + 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) .... [...] Cheers /Ilias