* [PATCH] tpm: check size of response before accessing data
@ 2017-01-05 12:11 Stefan Berger
[not found] ` <1483618284-3470-1-git-send-email-stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
0 siblings, 1 reply; 8+ messages in thread
From: Stefan Berger @ 2017-01-05 12:11 UTC (permalink / raw)
To: tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA
Cc: linux-security-module-u79uwXL29TY76Z2rM5mHXA
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] tpm: check size of response before accessingdata
[not found] ` <1483618284-3470-1-git-send-email-stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
@ 2017-01-05 12:14 ` Stefan Berger
2017-01-09 16:05 ` [PATCH] tpm: check size of response before accessing data Jarkko Sakkinen
1 sibling, 0 replies; 8+ messages in thread
From: Stefan Berger @ 2017-01-05 12:14 UTC (permalink / raw)
To: Stefan Berger
Cc: linux-security-module-u79uwXL29TY76Z2rM5mHXA,
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f
[-- 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] tpm: check size of response before accessing data
[not found] ` <1483618284-3470-1-git-send-email-stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
2017-01-05 12:14 ` [PATCH] tpm: check size of response before accessingdata Stefan Berger
@ 2017-01-09 16:05 ` Jarkko Sakkinen
[not found] ` <20170109160538.gwvksj253wl2v5oy-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
2017-01-09 18:09 ` Stefan Berger
1 sibling, 2 replies; 8+ messages in thread
From: Jarkko Sakkinen @ 2017-01-09 16:05 UTC (permalink / raw)
To: Stefan Berger
Cc: linux-security-module-u79uwXL29TY76Z2rM5mHXA,
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] tpm: check size of response before accessing data
[not found] ` <20170109160538.gwvksj253wl2v5oy-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
@ 2017-01-09 16:15 ` Jason Gunthorpe
0 siblings, 0 replies; 8+ messages in thread
From: Jason Gunthorpe @ 2017-01-09 16:15 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: linux-security-module-u79uwXL29TY76Z2rM5mHXA,
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] tpm: check size of response before accessing data
2017-01-09 16:05 ` [PATCH] tpm: check size of response before accessing data Jarkko Sakkinen
[not found] ` <20170109160538.gwvksj253wl2v5oy-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
@ 2017-01-09 18:09 ` Stefan Berger
2017-01-09 22:59 ` Jarkko Sakkinen
1 sibling, 1 reply; 8+ messages in thread
From: Stefan Berger @ 2017-01-09 18:09 UTC (permalink / raw)
To: Jarkko Sakkinen; +Cc: tpmdd-devel, linux-security-module
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] tpm: check size of response before accessing data
2017-01-09 18:09 ` Stefan Berger
@ 2017-01-09 22:59 ` Jarkko Sakkinen
2017-01-10 0:15 ` Stefan Berger
0 siblings, 1 reply; 8+ messages in thread
From: Jarkko Sakkinen @ 2017-01-09 22:59 UTC (permalink / raw)
To: Stefan Berger; +Cc: tpmdd-devel, linux-security-module
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] tpm: check size of response before accessing data
2017-01-09 22:59 ` Jarkko Sakkinen
@ 2017-01-10 0:15 ` Stefan Berger
2017-01-10 8:55 ` Jarkko Sakkinen
0 siblings, 1 reply; 8+ messages in thread
From: Stefan Berger @ 2017-01-10 0:15 UTC (permalink / raw)
To: Jarkko Sakkinen; +Cc: tpmdd-devel, linux-security-module
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] tpm: check size of response before accessing data
2017-01-10 0:15 ` Stefan Berger
@ 2017-01-10 8:55 ` Jarkko Sakkinen
0 siblings, 0 replies; 8+ messages in thread
From: Jarkko Sakkinen @ 2017-01-10 8:55 UTC (permalink / raw)
To: Stefan Berger; +Cc: tpmdd-devel, linux-security-module
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
> >
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-01-10 8:55 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-05 12:11 [PATCH] tpm: check size of response before accessing data Stefan Berger
[not found] ` <1483618284-3470-1-git-send-email-stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
2017-01-05 12:14 ` [PATCH] tpm: check size of response before accessingdata Stefan Berger
2017-01-09 16:05 ` [PATCH] tpm: check size of response before accessing data Jarkko Sakkinen
[not found] ` <20170109160538.gwvksj253wl2v5oy-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
2017-01-09 16:15 ` Jason Gunthorpe
2017-01-09 18:09 ` Stefan Berger
2017-01-09 22:59 ` Jarkko Sakkinen
2017-01-10 0:15 ` Stefan Berger
2017-01-10 8:55 ` Jarkko Sakkinen
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.