[PATCH] tpm: check size of response before accessing data

[PATCH] tpm: check size of response before accessing data

[Stefan Berger]

Check the size of the response before accesing data in
the response packet. This is to avoid accessing data beyond
the end of the response.

Signed-off-by: Stefan Berger <stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 drivers/char/tpm/tpm2-cmd.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index abaa355..98e591b 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -394,6 +394,10 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *out, size_t max)
 	(sizeof(struct tpm_input_header) + \
 	 sizeof(struct tpm2_get_tpm_pt_in))
 
+#define TPM2_GET_TPM_PT_OUT_SIZE \
+	(sizeof(struct tpm_output_header) + \
+	 sizeof(struct tpm2_get_tpm_pt_out))
+
 static const struct tpm_input_header tpm2_get_tpm_pt_header = {
 	.tag = cpu_to_be16(TPM2_ST_NO_SESSIONS),
 	.length = cpu_to_be32(TPM2_GET_TPM_PT_IN_SIZE),
@@ -713,6 +717,8 @@ ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id,  u32 *value,
 	cmd.params.get_tpm_pt_in.property_cnt = cpu_to_be32(1);
 
 	rc = tpm_transmit_cmd(chip, NULL, &cmd, sizeof(cmd), 0, desc);
+	if (be32_to_cpu(cmd.header.out.length) < TPM2_GET_TPM_PT_OUT_SIZE)
+		return -EFAULT;
 	if (!rc)
 		*value = be32_to_cpu(cmd.params.get_tpm_pt_out.value);
 
-- 
2.4.3


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH] tpm: check size of response before accessingdata

[Stefan Berger]

[-- Attachment #1.1: Type: text/plain, Size: 2181 bytes --]

Stefan Berger <stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> wrote on 01/05/2017 07:11:24 
AM:

> 
> Check the size of the response before accesing data in
> the response packet. This is to avoid accessing data beyond
> the end of the response.

This patch applies on top of Jarkko's tabrm tree.
There are of course many more places where such checks should be done, if 
we agree that we want them to be done.

    Stefan

> 
> Signed-off-by: Stefan Berger <stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> ---
>  drivers/char/tpm/tpm2-cmd.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index abaa355..98e591b 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -394,6 +394,10 @@ int tpm2_get_random(struct tpm_chip *chip, u8 
> *out, size_t max)
>     (sizeof(struct tpm_input_header) + \
>      sizeof(struct tpm2_get_tpm_pt_in))
> 
> +#define TPM2_GET_TPM_PT_OUT_SIZE \
> +   (sizeof(struct tpm_output_header) + \
> +    sizeof(struct tpm2_get_tpm_pt_out))
> +
>  static const struct tpm_input_header tpm2_get_tpm_pt_header = {
>     .tag = cpu_to_be16(TPM2_ST_NO_SESSIONS),
>     .length = cpu_to_be32(TPM2_GET_TPM_PT_IN_SIZE),
> @@ -713,6 +717,8 @@ ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, 
> u32 property_id,  u32 *value,
>     cmd.params.get_tpm_pt_in.property_cnt = cpu_to_be32(1);
> 
>     rc = tpm_transmit_cmd(chip, NULL, &cmd, sizeof(cmd), 0, desc);
> +   if (be32_to_cpu(cmd.header.out.length) < TPM2_GET_TPM_PT_OUT_SIZE)
> +      return -EFAULT;
>     if (!rc)
>        *value = be32_to_cpu(cmd.params.get_tpm_pt_out.value);
> 
> -- 
> 2.4.3
> 
> 
> 
------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most 
> engaging tech sites, SlashDot.org! http://sdm.link/slashdot
> _______________________________________________
> tpmdd-devel mailing list
> tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
> 



[-- Attachment #1.2: Type: text/html, Size: 2999 bytes --]

[-- Attachment #2: Type: text/plain, Size: 203 bytes --]

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

[-- Attachment #3: Type: text/plain, Size: 192 bytes --]

_______________________________________________
tpmdd-devel mailing list
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [PATCH] tpm: check size of response before accessing data

[Jarkko Sakkinen]

On Thu, Jan 05, 2017 at 07:11:24AM -0500, Stefan Berger wrote:
> Check the size of the response before accesing data in
> the response packet. This is to avoid accessing data beyond
> the end of the response.
> 
> Signed-off-by: Stefan Berger <stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>

How on earth this could happen if we request only one property?

/Jarkko

> ---
>  drivers/char/tpm/tpm2-cmd.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index abaa355..98e591b 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -394,6 +394,10 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *out, size_t max)
>  	(sizeof(struct tpm_input_header) + \
>  	 sizeof(struct tpm2_get_tpm_pt_in))
>  
> +#define TPM2_GET_TPM_PT_OUT_SIZE \
> +	(sizeof(struct tpm_output_header) + \
> +	 sizeof(struct tpm2_get_tpm_pt_out))
> +
>  static const struct tpm_input_header tpm2_get_tpm_pt_header = {
>  	.tag = cpu_to_be16(TPM2_ST_NO_SESSIONS),
>  	.length = cpu_to_be32(TPM2_GET_TPM_PT_IN_SIZE),
> @@ -713,6 +717,8 @@ ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id,  u32 *value,
>  	cmd.params.get_tpm_pt_in.property_cnt = cpu_to_be32(1);
>  
>  	rc = tpm_transmit_cmd(chip, NULL, &cmd, sizeof(cmd), 0, desc);
> +	if (be32_to_cpu(cmd.header.out.length) < TPM2_GET_TPM_PT_OUT_SIZE)
> +		return -EFAULT;
>  	if (!rc)
>  		*value = be32_to_cpu(cmd.params.get_tpm_pt_out.value);
>  
> -- 
> 2.4.3
> 

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH] tpm: check size of response before accessing data

[Jason Gunthorpe]

On Mon, Jan 09, 2017 at 06:05:38PM +0200, Jarkko Sakkinen wrote:
> On Thu, Jan 05, 2017 at 07:11:24AM -0500, Stefan Berger wrote:
> > Check the size of the response before accesing data in
> > the response packet. This is to avoid accessing data beyond
> > the end of the response.
> > 
> > Signed-off-by: Stefan Berger <stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> 
> How on earth this could happen if we request only one property?

His (software) TPM is broken.

Now that we have the vtpm stuff it is super-critical that the kernel
unmarshal path be bomb proof - it needs to treat the TPM itself as a
hostile entity.

You should look at all of it and make sure the proper bounds checks
are done, multiples can't overflow, and so forth.

Jason

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH] tpm: check size of response before accessing data

[Stefan Berger]

On 01/09/2017 11:05 AM, Jarkko Sakkinen wrote:
> On Thu, Jan 05, 2017 at 07:11:24AM -0500, Stefan Berger wrote:
>> Check the size of the response before accesing data in
>> the response packet. This is to avoid accessing data beyond
>> the end of the response.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> How on earth this could happen if we request only one property?

My test program vtpmctrl ( 
https://github.com/stefanberger/linux-vtpm-tests ) didn't feed the 
kernel a proper response to a TPM command and that's why this code blew 
up. We do have a very basic check in the driver and otherwise assume 
that the TPM is a trusted device responding with an expected response.

http://lxr.free-electrons.com/source/drivers/char/tpm/tpm-interface.c#L417

414         len = tpm_transmit(chip, (const u8 *)cmd, len, flags);
415         if (len <  0)
416                 return len;
417         else if (len < TPM_HEADER_SIZE)
418                 return -EFAULT;


Now should we expand on this check or just assume the device is flawless?

This particular check above should probably check whether the len is 
also what the len in the packet indicates.

    Stefan

Re: [PATCH] tpm: check size of response before accessing data

[Jarkko Sakkinen]

On Mon, Jan 09, 2017 at 01:09:31PM -0500, Stefan Berger wrote:
> On 01/09/2017 11:05 AM, Jarkko Sakkinen wrote:
> > On Thu, Jan 05, 2017 at 07:11:24AM -0500, Stefan Berger wrote:
> > > Check the size of the response before accesing data in
> > > the response packet. This is to avoid accessing data beyond
> > > the end of the response.
> > > 
> > > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> > How on earth this could happen if we request only one property?
> 
> My test program vtpmctrl ( https://github.com/stefanberger/linux-vtpm-tests
> ) didn't feed the kernel a proper response to a TPM command and that's why
> this code blew up. We do have a very basic check in the driver and otherwise
> assume that the TPM is a trusted device responding with an expected
> response.

Hmm.... I guess I could add this check but I'll have to probably
do a similar check at least in one other place in this patch set
where I grab the metadata for commands.

I guess similar issues will arise as the virtual TPMs get more
common. For now I think a good guideline is

1. For new code check that validation for message size is in place.
2. Fix the old code as you bump into issus.

/Jarkko

Re: [PATCH] tpm: check size of response before accessing data

[Stefan Berger]

On 01/09/2017 05:59 PM, Jarkko Sakkinen wrote:
> On Mon, Jan 09, 2017 at 01:09:31PM -0500, Stefan Berger wrote:
>> On 01/09/2017 11:05 AM, Jarkko Sakkinen wrote:
>>> On Thu, Jan 05, 2017 at 07:11:24AM -0500, Stefan Berger wrote:
>>>> Check the size of the response before accesing data in
>>>> the response packet. This is to avoid accessing data beyond
>>>> the end of the response.
>>>>
>>>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>> How on earth this could happen if we request only one property?
>> My test program vtpmctrl ( https://github.com/stefanberger/linux-vtpm-tests
>> ) didn't feed the kernel a proper response to a TPM command and that's why
>> this code blew up. We do have a very basic check in the driver and otherwise
>> assume that the TPM is a trusted device responding with an expected
>> response.
> Hmm.... I guess I could add this check but I'll have to probably
> do a similar check at least in one other place in this patch set
> where I grab the metadata for commands.
>
> I guess similar issues will arise as the virtual TPMs get more
> common. For now I think a good guideline is
>
> 1. For new code check that validation for message size is in place.

Before accessing data in the response, make sure we don't access beyond 
the number of bytes returned.

> 2. Fix the old code as you bump into issus.

It doesn't look too bad. I would rebase my current patch on your master 
tree and submit a few small other ones with it. Agrred?

    Stefan

>
> /Jarkko
>

Re: [PATCH] tpm: check size of response before accessing data

[Jarkko Sakkinen]

On Mon, Jan 09, 2017 at 07:15:29PM -0500, Stefan Berger wrote:
> On 01/09/2017 05:59 PM, Jarkko Sakkinen wrote:
> > On Mon, Jan 09, 2017 at 01:09:31PM -0500, Stefan Berger wrote:
> > > On 01/09/2017 11:05 AM, Jarkko Sakkinen wrote:
> > > > On Thu, Jan 05, 2017 at 07:11:24AM -0500, Stefan Berger wrote:
> > > > > Check the size of the response before accesing data in
> > > > > the response packet. This is to avoid accessing data beyond
> > > > > the end of the response.
> > > > > 
> > > > > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> > > > How on earth this could happen if we request only one property?
> > > My test program vtpmctrl ( https://github.com/stefanberger/linux-vtpm-tests
> > > ) didn't feed the kernel a proper response to a TPM command and that's why
> > > this code blew up. We do have a very basic check in the driver and otherwise
> > > assume that the TPM is a trusted device responding with an expected
> > > response.
> > Hmm.... I guess I could add this check but I'll have to probably
> > do a similar check at least in one other place in this patch set
> > where I grab the metadata for commands.
> > 
> > I guess similar issues will arise as the virtual TPMs get more
> > common. For now I think a good guideline is
> > 
> > 1. For new code check that validation for message size is in place.
> 
> Before accessing data in the response, make sure we don't access beyond the
> number of bytes returned.
> 
> > 2. Fix the old code as you bump into issus.
> 
> It doesn't look too bad. I would rebase my current patch on your master tree
> and submit a few small other ones with it. Agrred?

Hmm. Are you talking about stuff you are adding to the tpm2-space.c.
For me it is less trouble to add checks myself than applying 3rd party
patches while preparing the next patch set version. This is only
overhead for me and I will anyway would squash these checks to my
own commits.

> 
>    Stefan
> 

/Jarkko
> > 
> > /Jarkko
> > 
>