| Message ID | 1483618284-3470-1-git-send-email-stefanb@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
Stefan Berger <stefanb@linux.vnet.ibm.com> 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@linux.vnet.ibm.com> > --- > 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@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/tpmdd-devel > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
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 > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
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 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
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 ------------------------------------------------------------------------------ Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today. http://sdm.link/xeonphi
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 ------------------------------------------------------------------------------ Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today. http://sdm.link/xeonphi
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 > ------------------------------------------------------------------------------ Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today. http://sdm.link/xeonphi
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 > > > ------------------------------------------------------------------------------ Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today. http://sdm.link/xeonphi
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);
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(+)