diff mbox series

[v5,6/7] tpm: ensure that the output of PCR read contains the correct digest size

Message ID 20181114153108.12907-7-roberto.sassu@huawei.com (mailing list archive)
State New, archived
Headers show
Series tpm: retrieve digest size of unknown algorithms from TPM | expand

Commit Message

Roberto Sassu Nov. 14, 2018, 3:31 p.m. UTC 
This patch protects against data corruption that could happen in the bus,
by checking that that the digest size returned by the TPM during a PCR read
matches the size of the algorithm passed to tpm2_pcr_read().

This check is performed after information about the PCR banks has been
retrieved.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: stable@vger.kernel.org
---
 drivers/char/tpm/tpm2-cmd.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

Comments

Jarkko Sakkinen Nov. 16, 2018, 1:41 p.m. UTC | #1 
On Wed, Nov 14, 2018 at 04:31:07PM +0100, Roberto Sassu wrote:
> This patch protects against data corruption that could happen in the bus,
> by checking that that the digest size returned by the TPM during a PCR read
> matches the size of the algorithm passed to tpm2_pcr_read().
> 
> This check is performed after information about the PCR banks has been
> retrieved.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Cc: stable@vger.kernel.org

Missing fixes tag.

/Jarkko
Roberto Sassu Nov. 16, 2018, 4:06 p.m. UTC | #2 
On 11/16/2018 2:41 PM, Jarkko Sakkinen wrote:
> On Wed, Nov 14, 2018 at 04:31:07PM +0100, Roberto Sassu wrote:
>> This patch protects against data corruption that could happen in the bus,
>> by checking that that the digest size returned by the TPM during a PCR read
>> matches the size of the algorithm passed to tpm2_pcr_read().
>>
>> This check is performed after information about the PCR banks has been
>> retrieved.
>>
>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>> Cc: stable@vger.kernel.org
> 
> Missing fixes tag.

Before this patch set, tpm2_pcr_extend() always copied 20 bytes from the
output sent by the TPM.

Roberto


> /Jarkko
>
Jarkko Sakkinen Nov. 18, 2018, 7:32 a.m. UTC | #3 
On Fri, Nov 16, 2018 at 05:06:48PM +0100, Roberto Sassu wrote:
> On 11/16/2018 2:41 PM, Jarkko Sakkinen wrote:
> > On Wed, Nov 14, 2018 at 04:31:07PM +0100, Roberto Sassu wrote:
> > > This patch protects against data corruption that could happen in the bus,
> > > by checking that that the digest size returned by the TPM during a PCR read
> > > matches the size of the algorithm passed to tpm2_pcr_read().
> > > 
> > > This check is performed after information about the PCR banks has been
> > > retrieved.
> > > 
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > Cc: stable@vger.kernel.org
> > 
> > Missing fixes tag.
> 
> Before this patch set, tpm2_pcr_extend() always copied 20 bytes from the
> output sent by the TPM.
> 
> Roberto

Aah, right, of course. Well the patch set is ATM somewhat broken because
this would require a fixes tag that points to a patch insdie the patch
set.

Probably good way to fix the issue is to just merge this with the
earlier commit.

/Jarkko
Roberto Sassu Nov. 19, 2018, 8:14 a.m. UTC | #4 
On 11/18/2018 8:32 AM, Jarkko Sakkinen wrote:
> On Fri, Nov 16, 2018 at 05:06:48PM +0100, Roberto Sassu wrote:
>> On 11/16/2018 2:41 PM, Jarkko Sakkinen wrote:
>>> On Wed, Nov 14, 2018 at 04:31:07PM +0100, Roberto Sassu wrote:
>>>> This patch protects against data corruption that could happen in the bus,
>>>> by checking that that the digest size returned by the TPM during a PCR read
>>>> matches the size of the algorithm passed to tpm2_pcr_read().
>>>>
>>>> This check is performed after information about the PCR banks has been
>>>> retrieved.
>>>>
>>>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>>>> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>>>> Cc: stable@vger.kernel.org
>>>
>>> Missing fixes tag.
>>
>> Before this patch set, tpm2_pcr_extend() always copied 20 bytes from the
>> output sent by the TPM.
>>
>> Roberto
> 
> Aah, right, of course. Well the patch set is ATM somewhat broken because
> this would require a fixes tag that points to a patch insdie the patch
> set.
> 
> Probably good way to fix the issue is to just merge this with the
> earlier commit.

Unfortunately, it is not possible. The exact digest size has been
introduced with patch 5/7.

Roberto


> /Jarkko
>
Jarkko Sakkinen Nov. 19, 2018, 2:33 p.m. UTC | #5 
On Mon, Nov 19, 2018 at 09:14:00AM +0100, Roberto Sassu wrote:
> On 11/18/2018 8:32 AM, Jarkko Sakkinen wrote:
> > On Fri, Nov 16, 2018 at 05:06:48PM +0100, Roberto Sassu wrote:
> > > On 11/16/2018 2:41 PM, Jarkko Sakkinen wrote:
> > > > On Wed, Nov 14, 2018 at 04:31:07PM +0100, Roberto Sassu wrote:
> > > > > This patch protects against data corruption that could happen in the bus,
> > > > > by checking that that the digest size returned by the TPM during a PCR read
> > > > > matches the size of the algorithm passed to tpm2_pcr_read().
> > > > > 
> > > > > This check is performed after information about the PCR banks has been
> > > > > retrieved.
> > > > > 
> > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > > Cc: stable@vger.kernel.org
> > > > 
> > > > Missing fixes tag.
> > > 
> > > Before this patch set, tpm2_pcr_extend() always copied 20 bytes from the
> > > output sent by the TPM.
> > > 
> > > Roberto
> > 
> > Aah, right, of course. Well the patch set is ATM somewhat broken because
> > this would require a fixes tag that points to a patch insdie the patch
> > set.
> > 
> > Probably good way to fix the issue is to just merge this with the
> > earlier commit.
> 
> Unfortunately, it is not possible. The exact digest size has been
> introduced with patch 5/7.

I put this in simple terms: if I merge 5/7 the driver will be broken.
Any commit in the patch set should not leave the tree in broken state.

That implies that 5/7 and 6/7 cannot be separate commits.

/Jarkko
Roberto Sassu Nov. 19, 2018, 3:09 p.m. UTC | #6 
On 11/19/2018 3:33 PM, Jarkko Sakkinen wrote:
> On Mon, Nov 19, 2018 at 09:14:00AM +0100, Roberto Sassu wrote:
>> On 11/18/2018 8:32 AM, Jarkko Sakkinen wrote:
>>> On Fri, Nov 16, 2018 at 05:06:48PM +0100, Roberto Sassu wrote:
>>>> On 11/16/2018 2:41 PM, Jarkko Sakkinen wrote:
>>>>> On Wed, Nov 14, 2018 at 04:31:07PM +0100, Roberto Sassu wrote:
>>>>>> This patch protects against data corruption that could happen in the bus,
>>>>>> by checking that that the digest size returned by the TPM during a PCR read
>>>>>> matches the size of the algorithm passed to tpm2_pcr_read().
>>>>>>
>>>>>> This check is performed after information about the PCR banks has been
>>>>>> retrieved.
>>>>>>
>>>>>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>>>>>> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>>>>>> Cc: stable@vger.kernel.org
>>>>>
>>>>> Missing fixes tag.
>>>>
>>>> Before this patch set, tpm2_pcr_extend() always copied 20 bytes from the
>>>> output sent by the TPM.
>>>>
>>>> Roberto
>>>
>>> Aah, right, of course. Well the patch set is ATM somewhat broken because
>>> this would require a fixes tag that points to a patch insdie the patch
>>> set.
>>>
>>> Probably good way to fix the issue is to just merge this with the
>>> earlier commit.
>>
>> Unfortunately, it is not possible. The exact digest size has been
>> introduced with patch 5/7.
> 
> I put this in simple terms: if I merge 5/7 the driver will be broken.
> Any commit in the patch set should not leave the tree in broken state.

Patch 6/7 does not fix patch 5/7. The check in patch 6/7 is not done for
the PCR read performed by patch 5/7, because the digest sizes are not
yet available.

Roberto


> That implies that 5/7 and 6/7 cannot be separate commits.
> 
> /Jarkko
>
Jarkko Sakkinen Nov. 19, 2018, 4:07 p.m. UTC | #7 
On Mon, Nov 19, 2018 at 04:09:34PM +0100, Roberto Sassu wrote:
> On 11/19/2018 3:33 PM, Jarkko Sakkinen wrote:
> > On Mon, Nov 19, 2018 at 09:14:00AM +0100, Roberto Sassu wrote:
> > > On 11/18/2018 8:32 AM, Jarkko Sakkinen wrote:
> > > > On Fri, Nov 16, 2018 at 05:06:48PM +0100, Roberto Sassu wrote:
> > > > > On 11/16/2018 2:41 PM, Jarkko Sakkinen wrote:
> > > > > > On Wed, Nov 14, 2018 at 04:31:07PM +0100, Roberto Sassu wrote:
> > > > > > > This patch protects against data corruption that could happen in the bus,
> > > > > > > by checking that that the digest size returned by the TPM during a PCR read
> > > > > > > matches the size of the algorithm passed to tpm2_pcr_read().
> > > > > > > 
> > > > > > > This check is performed after information about the PCR banks has been
> > > > > > > retrieved.
> > > > > > > 
> > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > > > > Cc: stable@vger.kernel.org
> > > > > > 
> > > > > > Missing fixes tag.
> > > > > 
> > > > > Before this patch set, tpm2_pcr_extend() always copied 20 bytes from the
> > > > > output sent by the TPM.
> > > > > 
> > > > > Roberto
> > > > 
> > > > Aah, right, of course. Well the patch set is ATM somewhat broken because
> > > > this would require a fixes tag that points to a patch insdie the patch
> > > > set.
> > > > 
> > > > Probably good way to fix the issue is to just merge this with the
> > > > earlier commit.
> > > 
> > > Unfortunately, it is not possible. The exact digest size has been
> > > introduced with patch 5/7.
> > 
> > I put this in simple terms: if I merge 5/7 the driver will be broken.
> > Any commit in the patch set should not leave the tree in broken state.
> 
> Patch 6/7 does not fix patch 5/7. The check in patch 6/7 is not done for
> the PCR read performed by patch 5/7, because the digest sizes are not
> yet available.

Ah, right. Point taken :-)

/Jarkko
Roberto Sassu Nov. 28, 2018, 8:40 a.m. UTC | #8 
On 11/19/2018 5:07 PM, Jarkko Sakkinen wrote:
> On Mon, Nov 19, 2018 at 04:09:34PM +0100, Roberto Sassu wrote:
>> On 11/19/2018 3:33 PM, Jarkko Sakkinen wrote:
>>> On Mon, Nov 19, 2018 at 09:14:00AM +0100, Roberto Sassu wrote:
>>>> On 11/18/2018 8:32 AM, Jarkko Sakkinen wrote:
>>>>> On Fri, Nov 16, 2018 at 05:06:48PM +0100, Roberto Sassu wrote:
>>>>>> On 11/16/2018 2:41 PM, Jarkko Sakkinen wrote:
>>>>>>> On Wed, Nov 14, 2018 at 04:31:07PM +0100, Roberto Sassu wrote:
>>>>>>>> This patch protects against data corruption that could happen in the bus,
>>>>>>>> by checking that that the digest size returned by the TPM during a PCR read
>>>>>>>> matches the size of the algorithm passed to tpm2_pcr_read().
>>>>>>>>
>>>>>>>> This check is performed after information about the PCR banks has been
>>>>>>>> retrieved.
>>>>>>>>
>>>>>>>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>>>>>>>> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>>>>>>>> Cc: stable@vger.kernel.org
>>>>>>>
>>>>>>> Missing fixes tag.
>>>>>>
>>>>>> Before this patch set, tpm2_pcr_extend() always copied 20 bytes from the
>>>>>> output sent by the TPM.
>>>>>>
>>>>>> Roberto
>>>>>
>>>>> Aah, right, of course. Well the patch set is ATM somewhat broken because
>>>>> this would require a fixes tag that points to a patch insdie the patch
>>>>> set.
>>>>>
>>>>> Probably good way to fix the issue is to just merge this with the
>>>>> earlier commit.
>>>>
>>>> Unfortunately, it is not possible. The exact digest size has been
>>>> introduced with patch 5/7.
>>>
>>> I put this in simple terms: if I merge 5/7 the driver will be broken.
>>> Any commit in the patch set should not leave the tree in broken state.
>>
>> Patch 6/7 does not fix patch 5/7. The check in patch 6/7 is not done for
>> the PCR read performed by patch 5/7, because the digest sizes are not
>> yet available.
> 
> Ah, right. Point taken :-)

Should I keep CC: stable@vger.kernel.org?

Roberto


> /Jarkko
>
Jarkko Sakkinen Nov. 28, 2018, 7:19 p.m. UTC | #9 
On Wed, Nov 28, 2018 at 09:40:37AM +0100, Roberto Sassu wrote:
> On 11/19/2018 5:07 PM, Jarkko Sakkinen wrote:
> > On Mon, Nov 19, 2018 at 04:09:34PM +0100, Roberto Sassu wrote:
> > > On 11/19/2018 3:33 PM, Jarkko Sakkinen wrote:
> > > > On Mon, Nov 19, 2018 at 09:14:00AM +0100, Roberto Sassu wrote:
> > > > > On 11/18/2018 8:32 AM, Jarkko Sakkinen wrote:
> > > > > > On Fri, Nov 16, 2018 at 05:06:48PM +0100, Roberto Sassu wrote:
> > > > > > > On 11/16/2018 2:41 PM, Jarkko Sakkinen wrote:
> > > > > > > > On Wed, Nov 14, 2018 at 04:31:07PM +0100, Roberto Sassu wrote:
> > > > > > > > > This patch protects against data corruption that could happen in the bus,
> > > > > > > > > by checking that that the digest size returned by the TPM during a PCR read
> > > > > > > > > matches the size of the algorithm passed to tpm2_pcr_read().
> > > > > > > > > 
> > > > > > > > > This check is performed after information about the PCR banks has been
> > > > > > > > > retrieved.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > > > > > > Cc: stable@vger.kernel.org
> > > > > > > > 
> > > > > > > > Missing fixes tag.
> > > > > > > 
> > > > > > > Before this patch set, tpm2_pcr_extend() always copied 20 bytes from the
> > > > > > > output sent by the TPM.
> > > > > > > 
> > > > > > > Roberto
> > > > > > 
> > > > > > Aah, right, of course. Well the patch set is ATM somewhat broken because
> > > > > > this would require a fixes tag that points to a patch insdie the patch
> > > > > > set.
> > > > > > 
> > > > > > Probably good way to fix the issue is to just merge this with the
> > > > > > earlier commit.
> > > > > 
> > > > > Unfortunately, it is not possible. The exact digest size has been
> > > > > introduced with patch 5/7.
> > > > 
> > > > I put this in simple terms: if I merge 5/7 the driver will be broken.
> > > > Any commit in the patch set should not leave the tree in broken state.
> > > 
> > > Patch 6/7 does not fix patch 5/7. The check in patch 6/7 is not done for
> > > the PCR read performed by patch 5/7, because the digest sizes are not
> > > yet available.
> > 
> > Ah, right. Point taken :-)
> 
> Should I keep CC: stable@vger.kernel.org?

Nope, my bad, sorry.

/Jarkko
diff mbox series

Patch

diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index acaaab72ef2e..974465f04b78 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -179,15 +179,29 @@  struct tpm2_pcr_read_out {
 int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
 		  struct tpm_digest *digest_struct, u16 *digest_size_ptr)
 {
+	int i;
 	int rc;
 	struct tpm_buf buf;
 	struct tpm2_pcr_read_out *out;
 	u8 pcr_select[TPM2_PCR_SELECT_MIN] = {0};
 	u16 digest_size;
+	u16 expected_digest_size = 0;
 
 	if (pcr_idx >= TPM2_PLATFORM_PCR)
 		return -EINVAL;
 
+	if (!digest_size_ptr) {
+		for (i = 0; i < chip->nr_allocated_banks &&
+		     chip->allocated_banks[i].alg_id != digest_struct->alg_id;
+		     i++)
+			;
+
+		if (i == chip->nr_allocated_banks)
+			return -EINVAL;
+
+		expected_digest_size = chip->allocated_banks[i].digest_size;
+	}
+
 	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_PCR_READ);
 	if (rc)
 		return rc;
@@ -207,7 +221,8 @@  int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
 
 	out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE];
 	digest_size = be16_to_cpu(out->digest_size);
-	if (digest_size > sizeof(digest_struct->digest)) {
+	if (digest_size > sizeof(digest_struct->digest) ||
+	    (!digest_size_ptr && digest_size != expected_digest_size)) {
 		rc = -EINVAL;
 		goto out;
 	}