tpm: check size of response before accessing data
diff mbox

Message ID 1483618284-3470-1-git-send-email-stefanb@linux.vnet.ibm.com
State New
Headers show

Commit Message

Stefan Berger Jan. 5, 2017, 12:11 p.m. UTC
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>
---
 drivers/char/tpm/tpm2-cmd.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Jarkko Sakkinen Jan. 9, 2017, 4:05 p.m. UTC | #1
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?

/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
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jason Gunthorpe Jan. 9, 2017, 4:15 p.m. UTC | #2
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@linux.vnet.ibm.com>
> 
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stefan Berger Jan. 9, 2017, 6:09 p.m. UTC | #3
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

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarkko Sakkinen Jan. 9, 2017, 10:59 p.m. UTC | #4
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stefan Berger Jan. 10, 2017, 12:15 a.m. UTC | #5
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
>

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarkko Sakkinen Jan. 10, 2017, 8:55 a.m. UTC | #6
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
> > 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

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);