From: Roberto Sassu <roberto.sassu@huawei.com>
To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: <zohar@linux.ibm.com>, <david.safford@ge.com>,
<monty.wiseman@ge.com>, <linux-integrity@vger.kernel.org>,
<linux-security-module@vger.kernel.org>,
<linux-kernel@vger.kernel.org>, <silviu.vlasceanu@huawei.com>,
"Matthew Garrett" <matthewgarrett@google.com>
Subject: Re: [PATCH v7 5/5] tpm: pass an array of tpm_extend_digest structures to tpm_pcr_extend()
Date: Mon, 21 Jan 2019 10:58:22 +0100 [thread overview]
Message-ID: <de1cd369-c06c-0107-80cc-fe3967799849@huawei.com> (raw)
In-Reply-To: <20190118151253.GH4080@linux.intel.com>
On 1/18/2019 4:12 PM, Jarkko Sakkinen wrote:
> On Thu, Jan 17, 2019 at 08:59:00AM +0100, Roberto Sassu wrote:
>> On 12/20/2018 4:21 PM, Jarkko Sakkinen wrote:
>>> On Thu, Dec 13, 2018 at 11:29:45AM +0100, Roberto Sassu wrote:
>>>> Currently, tpm_pcr_extend() accepts as an input only a SHA1 digest.
>>>>
>>>> This patch modifies the definition of tpm_pcr_extend() to allow other
>>>> kernel subsystems to pass a digest for each algorithm supported by the TPM.
>>>> All digests are processed by the TPM in one operation.
>>>>
>>>> If a tpm_pcr_extend() caller provides a subset of the supported algorithms,
>>>> the TPM driver extends the remaining PCR banks with the first digest
>>>> passed as an argument to the function.
>>>>
>>>> The new tpm_extend digest structure has been preferred to the tpm_digest
>>>> structure, to let the caller specify the size of the digest (which may be
>>>> unknown to the TPM driver).
>>>>
>>>> Due to the API change, ima_pcr_extend() and pcrlock() have been modified.
>>>>
>>>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>>>> ---
>>>> drivers/char/tpm/tpm-interface.c | 24 +++++---------------
>>>> drivers/char/tpm/tpm.h | 5 +++--
>>>> drivers/char/tpm/tpm1-cmd.c | 13 ++++++++---
>>>> drivers/char/tpm/tpm2-cmd.c | 35 +++++++++++++++++++++---------
>>>> include/linux/tpm.h | 13 ++++++++---
>>>> security/integrity/ima/ima_queue.c | 5 ++++-
>>>> security/keys/trusted.c | 5 ++++-
>>>> 7 files changed, 62 insertions(+), 38 deletions(-)
>>>>
>>>> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>>>> index eb7c79ca8a94..911fea19e408 100644
>>>> --- a/drivers/char/tpm/tpm-interface.c
>>>> +++ b/drivers/char/tpm/tpm-interface.c
>>>> @@ -478,42 +478,30 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read);
>>>> * tpm_pcr_extend - extend a PCR value in SHA1 bank.
>>>> * @chip: a &struct tpm_chip instance, %NULL for the default chip
>>>> * @pcr_idx: the PCR to be retrieved
>>>> - * @hash: the hash value used to extend the PCR value
>>>> + * @count: number of tpm_extend_digest structures
>>>> + * @digests: array of tpm_extend_digest structures used to extend PCRs
>>>> *
>>>> * Note: with TPM 2.0 extends also those banks for which no digest was
>>>> * specified in order to prevent malicious use of those PCR banks.
>>>> *
>>>> * Return: same as with tpm_transmit_cmd()
>>>> */
>>>> -int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash)
>>>> +int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>>>> + const struct tpm_extend_digest *digests)
>>>
>>> Remove const. Document how @digests is used like the special meaning
>>> of the first index. I faintly remember asking this last time.
>>>
>>>> {
>>>> int rc;
>>>> - struct tpm_digest *digest_list;
>>>> - int i;
>>>> chip = tpm_find_get_ops(chip);
>>>> if (!chip)
>>>> return -ENODEV;
>>>> if (chip->flags & TPM_CHIP_FLAG_TPM2) {
>>>> - digest_list = kcalloc(chip->nr_allocated_banks,
>>>> - sizeof(*digest_list), GFP_KERNEL);
>>>> - if (!digest_list)
>>>> - return -ENOMEM;
>>>> -
>>>> - for (i = 0; i < chip->nr_allocated_banks; i++) {
>>>> - digest_list[i].alg_id = chip->allocated_banks[i].alg_id;
>>>> - memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
>>>> - }
>>>> -
>>>> - rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_allocated_banks,
>>>> - digest_list);
>>>> - kfree(digest_list);
>>>> + rc = tpm2_pcr_extend(chip, pcr_idx, count, digests);
>>>> tpm_put_ops(chip);
>>>> return rc;
>>>> }
>>>> - rc = tpm1_pcr_extend(chip, pcr_idx, hash,
>>>> + rc = tpm1_pcr_extend(chip, pcr_idx, count, digests,
>>>> "attempting extend a PCR value");
>>>
>>> The validation is missing that the provided array has only one element
>>> and the algorithm is SHA1. Could be done also inside tpm1_pcr_extend()
>>> but what you are doing to that function does not make any sense so
>>> better to do it here.
>>>
>>>> tpm_put_ops(chip);
>>>> return rc;
>>>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>>>> index 64d93d26087f..6b446504d2fe 100644
>>>> --- a/drivers/char/tpm/tpm.h
>>>> +++ b/drivers/char/tpm/tpm.h
>>>> @@ -504,7 +504,8 @@ int tpm1_auto_startup(struct tpm_chip *chip);
>>>> int tpm1_do_selftest(struct tpm_chip *chip);
>>>> int tpm1_get_timeouts(struct tpm_chip *chip);
>>>> unsigned long tpm1_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
>>>> -int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash,
>>>> +int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>>>> + const struct tpm_extend_digest *digests,
>>>> const char *log_msg);
>>>> int tpm1_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf);
>>>> ssize_t tpm1_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap,
>>>> @@ -551,7 +552,7 @@ int tpm2_get_timeouts(struct tpm_chip *chip);
>>>> int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
>>>> struct tpm_digest *digest, u16 *digest_size_ptr);
>>>> int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>>>> - struct tpm_digest *digests);
>>>> + const struct tpm_extend_digest *digests);
>>>> int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max);
>>>> void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle,
>>>> unsigned int flags);
>>>> diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
>>>> index 8b70a7f884a7..04ee10284b8c 100644
>>>> --- a/drivers/char/tpm/tpm1-cmd.c
>>>> +++ b/drivers/char/tpm/tpm1-cmd.c
>>>> @@ -449,12 +449,20 @@ int tpm1_get_timeouts(struct tpm_chip *chip)
>>>> }
>>>> #define TPM_ORD_PCR_EXTEND 20
>>>> -int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash,
>>>> +int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>>>> + const struct tpm_extend_digest *digests,
>>>> const char *log_msg)
>>>> {
>>>> struct tpm_buf buf;
>>>> + u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 };
>>>> + const u8 *hash;
>>>> int rc;
>>>> + hash = dummy_hash;
>>>> + if (count)
>>>> + memcpy(dummy_hash, digests[0].data,
>>>> + min(digests[0].size, (u16)sizeof(dummy_hash)));
>>>> +
>>>
>>> You copy memory from one place to another without any good reason to do
>>> so. My suggestion is just not to change tpm1_pcr_extend() at all.
>>>
>>>> rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_PCR_EXTEND);
>>>> if (rc)
>>>> return rc;
>>>> @@ -743,7 +751,6 @@ int tpm1_auto_startup(struct tpm_chip *chip)
>>>> */
>>>> int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr)
>>>> {
>>>> - u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 };
>>>> struct tpm_buf buf;
>>>> unsigned int try;
>>>> int rc;
>>>> @@ -751,7 +758,7 @@ int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr)
>>>> /* for buggy tpm, flush pcrs with extend to selected dummy */
>>>> if (tpm_suspend_pcr)
>>>> - rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, dummy_hash,
>>>> + rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, 0, NULL,
>>>> "extending dummy pcr before suspend");
>>>> rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_SAVESTATE);
>>>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>>>> index 6ce5173cf0e5..77b5808270c6 100644
>>>> --- a/drivers/char/tpm/tpm2-cmd.c
>>>> +++ b/drivers/char/tpm/tpm2-cmd.c
>>>> @@ -247,21 +247,22 @@ struct tpm2_null_auth_area {
>>>> *
>>>> * @chip: TPM chip to use.
>>>> * @pcr_idx: index of the PCR.
>>>> - * @count: number of digests passed.
>>>> - * @digests: list of pcr banks and corresponding digest values to extend.
>>>> + * @count: number of tpm_extend_digest passed.
>>>> + * @digests: array of tpm_extend_digest with digest values to extend.
>>>> *
>>>> * Return: Same as with tpm_transmit_cmd.
>>>> */
>>>
>>> The documentation about @digests.
>>>
>>>> int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>>>> - struct tpm_digest *digests)
>>>> + const struct tpm_extend_digest *digests)
>>>> {
>>>> struct tpm_buf buf;
>>>> struct tpm2_null_auth_area auth_area;
>>>> + const struct tpm_extend_digest *digest;
>>>> + u8 dummy_hash[SHA512_DIGEST_SIZE] = { 0 };
>>>> + const u8 *hash;
>>>> int rc;
>>>> int i;
>>>> -
>>>> - if (count > chip->nr_allocated_banks)
>>>> - return -EINVAL;
>>>> + int j;
>>>> rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
>>>> if (rc)
>>>> @@ -277,11 +278,25 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>>>> tpm_buf_append_u32(&buf, sizeof(struct tpm2_null_auth_area));
>>>> tpm_buf_append(&buf, (const unsigned char *)&auth_area,
>>>> sizeof(auth_area));
>>>> - tpm_buf_append_u32(&buf, count);
>>>> + tpm_buf_append_u32(&buf, chip->nr_allocated_banks);
>>>> +
>>>> + if (count)
>>>> + memcpy(dummy_hash, digests[0].data, digests[0].size);
>>>> +
>>>> + for (i = 0; i < chip->nr_allocated_banks; i++) {
>>>> + tpm_buf_append_u16(&buf, chip->allocated_banks[i].alg_id);
>>>> +
>>>> + hash = dummy_hash;
>>>> + for (j = 0; j < count; j++) {
>>>> + digest = digests + j;
>>>> +
>>>> + if (digest->alg_id == chip->allocated_banks[i].alg_id) {
>>>> + hash = digest->data;
>>>> + break;
>>>
>>> I think the whole design is just wrong. I did re-read your response to
>>> v6 again and I'm very sorry, but I just don't get this. Caller has all
>>> the information (from struct tpm_chip) to give the correct data. This
>>> function should validate that data (check algorithm ID and that's it).
>>
>> The question is if checking tpm->allocated_banks is a strict
>> requirement, or we can allow callers to use the algorithm they are
>> currently using, without further modifications.
>
> If you want to know what is available, you can use that array.
>
>>> Extending with the dummy hash should be done by the caller when
>>> preparing the array, not baked into this function. This kind of also
>>> makes obvious that we don't need this new struct. There should never be
>>> a local variable (whose size is BTW randomly chosen) called dummy_hash.
>>
>> This means duplicating the code for each caller. Currently, this work is
>> done by the TPM driver.
>
> It is better than fix the usage pattern like this. Not only that, but
> this somewhat complicated behavior now completely undocumented in the
> function description.
>
> You are making strong assumptions how some other subsystem might use
> extend operation. I will rather accept redundancy and consider
> consolidation later when there are two clients. Rather keep the TPM
> provided function simple and stupid.
I just found that your requirement does not match Mimi's requirements:
---
> The caller of tpm_pcr_extend() could pass a subset of the allocated
> banks, but the TPM driver extends all banks as before.
Agreed, there should be a clear division.
1) The caller shouldn't need to know anything about the chip->info.
2) The TPM driver should not rely on the caller to supply all the
hashes, but verify that all allocated banks are being extended.
---
See https://lkml.org/lkml/2018/11/19/603.
Also, if callers should be able to retrieve the list of supported
algorithms from the TPM, I should move the tpm_chip definition to
include/linux/tpm.h or introduce a new function that returns them.
However, you said that this shouldn't be done now:
---
> > > @@ -1048,16 +1047,22 @@ int tpm_pcr_extend(struct tpm_chip *chip,
int pcr_idx, const u8 *hash)
> > > return -ENODEV;
> >
> > Should take digest_list as input. Probably callers could re-use the
> > same digest_list array multiple times?
> >
> > Move struct tpm_chip to include/linux/tpm.h so that the caller can
query
> > nr_active_banks and active_banks and can create the digest array by
> > itself. Lets do this right at once now that this is being restructured.
>
> I have to move also other structures and #define. Wouldn't be better to
> introduce a new function to pass to the caller active_banks and
> nr_active_banks?
Revisited. I think it is fine how it is for now and we reconsider
later. Only thing I want to remark is that use should use kcalloc()
instead of kalloc_array() + memset().
---
See https://lkml.org/lkml/2018/11/13/1059.
How we should proceed?
Thanks
Roberto
> /Jarkko
>
--
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI
next prev parent reply other threads:[~2019-01-21 9:58 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-13 10:29 [PATCH v7 0/5] tpm: retrieve digest size of unknown algorithms from TPM Roberto Sassu
2018-12-13 10:29 ` [PATCH v7 1/5] tpm: dynamically allocate the allocated_banks array Roberto Sassu
2018-12-20 14:55 ` Jarkko Sakkinen
2018-12-21 9:40 ` Roberto Sassu
2018-12-22 0:03 ` Jarkko Sakkinen
2019-01-07 10:06 ` Roberto Sassu
2019-01-10 17:38 ` Jarkko Sakkinen
2019-01-11 7:53 ` Roberto Sassu
2019-01-11 16:35 ` Jarkko Sakkinen
2018-12-13 10:29 ` [PATCH v7 2/5] tpm: add _head suffix to tcg_efi_specid_event and tcg_pcr_event2 Roberto Sassu
2018-12-13 10:29 ` [PATCH v7 3/5] tpm: rename and export tpm2_digest and tpm2_algorithms Roberto Sassu
2018-12-13 10:29 ` [PATCH v7 4/5] tpm: retrieve digest size of unknown algorithms with PCR read Roberto Sassu
2018-12-13 10:29 ` [PATCH v7 5/5] tpm: pass an array of tpm_extend_digest structures to tpm_pcr_extend() Roberto Sassu
2018-12-20 15:21 ` Jarkko Sakkinen
2019-01-17 7:59 ` Roberto Sassu
2019-01-18 15:12 ` Jarkko Sakkinen
2019-01-21 8:11 ` Roberto Sassu
2019-01-21 12:30 ` Jarkko Sakkinen
2019-01-21 9:58 ` Roberto Sassu [this message]
2019-01-21 12:37 ` Jarkko Sakkinen
2019-01-21 13:50 ` Roberto Sassu
2019-01-22 16:55 ` Jarkko Sakkinen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=de1cd369-c06c-0107-80cc-fe3967799849@huawei.com \
--to=roberto.sassu@huawei.com \
--cc=david.safford@ge.com \
--cc=jarkko.sakkinen@linux.intel.com \
--cc=linux-integrity@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=matthewgarrett@google.com \
--cc=monty.wiseman@ge.com \
--cc=silviu.vlasceanu@huawei.com \
--cc=zohar@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.