| Message ID | 20190401190607.GA23795@beast (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] tpm: Actually fail on TPM errors during "get random" | expand |
On Mon, Apr 01, 2019 at 12:06:07PM -0700, Kees Cook wrote: > A "get random" may fail with a TPM error, but those codes were returned > as-is to the caller, which assumed the result was the number of bytes > that had been written to the target buffer, which could lead to a kernel > heap memory exposure and over-read. > > This fixes tpm1_get_random() to mask positive TPM errors into -EIO, as > before. > > [ 18.092103] tpm tpm0: A TPM error (379) occurred attempting get random > [ 18.092106] usercopy: Kernel memory exposure attempt detected from SLUB object 'kmalloc-64' (offset 0, size 379)! > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=1650989 > Reported-by: Phil Baker <baker1tex@gmail.com> > Reported-by: Craig Robson <craig@zhatt.com> > Fixes: 7aee9c52d7ac ("tpm: tpm1: rewrite tpm1_get_random() using tpm_buf structure") > Cc: Laura Abbott <labbott@redhat.com> > Cc: Tomas Winkler <tomas.winkler@intel.com> > Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > Cc: stable@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> > v3: fix never-succeed, limit checks to tpm cmd return (James, Jason) > v2: also fix tpm2 implementation (Jason Gunthorpe) > drivers/char/tpm/tpm1-cmd.c | 7 +++++-- > drivers/char/tpm/tpm2-cmd.c | 7 +++++-- > 2 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c > index 85dcf2654d11..faacbe1ffa1a 100644 > +++ b/drivers/char/tpm/tpm1-cmd.c > @@ -510,7 +510,7 @@ struct tpm1_get_random_out { > * > * Return: > * * number of bytes read > - * * -errno or a TPM return code otherwise > + * * -errno (positive TPM return codes are masked to -EIO) > */ > int tpm1_get_random(struct tpm_chip *chip, u8 *dest, size_t max) > { > @@ -531,8 +531,11 @@ int tpm1_get_random(struct tpm_chip *chip, u8 *dest, size_t max) > > rc = tpm_transmit_cmd(chip, &buf, sizeof(out->rng_data_len), > "attempting get random"); > - if (rc) > + if (rc) { > + if (rc > 0) > + rc = -EIO; > goto out; > + } > > out = (struct tpm1_get_random_out *)&buf.data[TPM_HEADER_SIZE]; > > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c > index e74c5b7b64bf..8ffa6af61580 100644 > +++ b/drivers/char/tpm/tpm2-cmd.c > @@ -301,7 +301,7 @@ struct tpm2_get_random_out { > * > * Return: > * size of the buffer on success, > - * -errno otherwise > + * -errno otherwise ((positive TPM return codes are masked to -EIO) Extra bracket, but otherwise looks fine to me Jason
On Mon, Apr 1, 2019 at 12:09 PM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > + * -errno otherwise ((positive TPM return codes are masked to -EIO) > > Extra bracket, but otherwise looks fine to me Thanks! (And I guess it's time for lunch so I can take a break and learn how to type again...)
> > A "get random" may fail with a TPM error, but those codes were returned as-is > to the caller, which assumed the result was the number of bytes that had been > written to the target buffer, which could lead to a kernel heap memory > exposure and over-read. > > This fixes tpm1_get_random() to mask positive TPM errors into -EIO, as before. > > [ 18.092103] tpm tpm0: A TPM error (379) occurred attempting get random > [ 18.092106] usercopy: Kernel memory exposure attempt detected from SLUB > object 'kmalloc-64' (offset 0, size 379)! > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=1650989 > Reported-by: Phil Baker <baker1tex@gmail.com> > Reported-by: Craig Robson <craig@zhatt.com> > Fixes: 7aee9c52d7ac ("tpm: tpm1: rewrite tpm1_get_random() using tpm_buf > structure") > Cc: Laura Abbott <labbott@redhat.com> > Cc: Tomas Winkler <tomas.winkler@intel.com> > Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > Cc: stable@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > v3: fix never-succeed, limit checks to tpm cmd return (James, Jason) > v2: also fix tpm2 implementation (Jason Gunthorpe) Looks good to me. Thanks Tomas > --- > drivers/char/tpm/tpm1-cmd.c | 7 +++++-- drivers/char/tpm/tpm2-cmd.c | 7 > +++++-- > 2 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c > index 85dcf2654d11..faacbe1ffa1a 100644 > --- a/drivers/char/tpm/tpm1-cmd.c > +++ b/drivers/char/tpm/tpm1-cmd.c > @@ -510,7 +510,7 @@ struct tpm1_get_random_out { > * > * Return: > * * number of bytes read > - * * -errno or a TPM return code otherwise > + * * -errno (positive TPM return codes are masked to -EIO) > */ > int tpm1_get_random(struct tpm_chip *chip, u8 *dest, size_t max) { @@ - > 531,8 +531,11 @@ int tpm1_get_random(struct tpm_chip *chip, u8 *dest, > size_t max) > > rc = tpm_transmit_cmd(chip, &buf, sizeof(out->rng_data_len), > "attempting get random"); > - if (rc) > + if (rc) { > + if (rc > 0) > + rc = -EIO; > goto out; > + } > > out = (struct tpm1_get_random_out > *)&buf.data[TPM_HEADER_SIZE]; > > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c > index e74c5b7b64bf..8ffa6af61580 100644 > --- a/drivers/char/tpm/tpm2-cmd.c > +++ b/drivers/char/tpm/tpm2-cmd.c > @@ -301,7 +301,7 @@ struct tpm2_get_random_out { > * > * Return: > * size of the buffer on success, > - * -errno otherwise > + * -errno otherwise ((positive TPM return codes are masked to -EIO) > */ > int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max) { @@ - > 328,8 +328,11 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, > size_t max) > offsetof(struct tpm2_get_random_out, > buffer), > "attempting get random"); > - if (err) > + if (err) { > + if (err > 0) > + err = -EIO; > goto out; > + } > > out = (struct tpm2_get_random_out *) > &buf.data[TPM_HEADER_SIZE]; > -- > 2.17.1 > > > -- > Kees Cook
On Mon, Apr 01, 2019 at 12:06:07PM -0700, Kees Cook wrote: > A "get random" may fail with a TPM error, but those codes were returned > as-is to the caller, which assumed the result was the number of bytes > that had been written to the target buffer, which could lead to a kernel > heap memory exposure and over-read. > > This fixes tpm1_get_random() to mask positive TPM errors into -EIO, as > before. > > [ 18.092103] tpm tpm0: A TPM error (379) occurred attempting get random > [ 18.092106] usercopy: Kernel memory exposure attempt detected from SLUB object 'kmalloc-64' (offset 0, size 379)! > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=1650989 > Reported-by: Phil Baker <baker1tex@gmail.com> > Reported-by: Craig Robson <craig@zhatt.com> > Fixes: 7aee9c52d7ac ("tpm: tpm1: rewrite tpm1_get_random() using tpm_buf structure") > Cc: Laura Abbott <labbott@redhat.com> > Cc: Tomas Winkler <tomas.winkler@intel.com> > Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > Cc: stable@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > v3: fix never-succeed, limit checks to tpm cmd return (James, Jason) > v2: also fix tpm2 implementation (Jason Gunthorpe) > --- > drivers/char/tpm/tpm1-cmd.c | 7 +++++-- > drivers/char/tpm/tpm2-cmd.c | 7 +++++-- > 2 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c > index 85dcf2654d11..faacbe1ffa1a 100644 > --- a/drivers/char/tpm/tpm1-cmd.c > +++ b/drivers/char/tpm/tpm1-cmd.c > @@ -510,7 +510,7 @@ struct tpm1_get_random_out { > * > * Return: > * * number of bytes read > - * * -errno or a TPM return code otherwise > + * * -errno (positive TPM return codes are masked to -EIO) > */ > int tpm1_get_random(struct tpm_chip *chip, u8 *dest, size_t max) > { > @@ -531,8 +531,11 @@ int tpm1_get_random(struct tpm_chip *chip, u8 *dest, size_t max) > > rc = tpm_transmit_cmd(chip, &buf, sizeof(out->rng_data_len), > "attempting get random"); > - if (rc) > + if (rc) { > + if (rc > 0) > + rc = -EIO; > goto out; > + } > > out = (struct tpm1_get_random_out *)&buf.data[TPM_HEADER_SIZE]; > > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c > index e74c5b7b64bf..8ffa6af61580 100644 > --- a/drivers/char/tpm/tpm2-cmd.c > +++ b/drivers/char/tpm/tpm2-cmd.c > @@ -301,7 +301,7 @@ struct tpm2_get_random_out { > * > * Return: > * size of the buffer on success, > - * -errno otherwise > + * -errno otherwise ((positive TPM return codes are masked to -EIO) > */ > int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max) > { > @@ -328,8 +328,11 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max) > offsetof(struct tpm2_get_random_out, > buffer), > "attempting get random"); > - if (err) > + if (err) { > + if (err > 0) > + err = -EIO; > goto out; > + } > > out = (struct tpm2_get_random_out *) > &buf.data[TPM_HEADER_SIZE]; > -- > 2.17.1 > > > -- > Kees Cook Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> /Jarkko
On Tue, Apr 02, 2019 at 02:46:25AM +0300, Jarkko Sakkinen wrote: > On Mon, Apr 01, 2019 at 12:06:07PM -0700, Kees Cook wrote: > > A "get random" may fail with a TPM error, but those codes were returned > > as-is to the caller, which assumed the result was the number of bytes > > that had been written to the target buffer, which could lead to a kernel > > heap memory exposure and over-read. > > > > This fixes tpm1_get_random() to mask positive TPM errors into -EIO, as > > before. > > > > [ 18.092103] tpm tpm0: A TPM error (379) occurred attempting get random > > [ 18.092106] usercopy: Kernel memory exposure attempt detected from SLUB object 'kmalloc-64' (offset 0, size 379)! > > > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=1650989 > > Reported-by: Phil Baker <baker1tex@gmail.com> > > Reported-by: Craig Robson <craig@zhatt.com> > > Fixes: 7aee9c52d7ac ("tpm: tpm1: rewrite tpm1_get_random() using tpm_buf structure") > > Cc: Laura Abbott <labbott@redhat.com> > > Cc: Tomas Winkler <tomas.winkler@intel.com> > > Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > Cc: stable@vger.kernel.org > > Signed-off-by: Kees Cook <keescook@chromium.org> > > --- > > v3: fix never-succeed, limit checks to tpm cmd return (James, Jason) > > v2: also fix tpm2 implementation (Jason Gunthorpe) > > --- > > drivers/char/tpm/tpm1-cmd.c | 7 +++++-- > > drivers/char/tpm/tpm2-cmd.c | 7 +++++-- > > 2 files changed, 10 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c > > index 85dcf2654d11..faacbe1ffa1a 100644 > > --- a/drivers/char/tpm/tpm1-cmd.c > > +++ b/drivers/char/tpm/tpm1-cmd.c > > @@ -510,7 +510,7 @@ struct tpm1_get_random_out { > > * > > * Return: > > * * number of bytes read > > - * * -errno or a TPM return code otherwise > > + * * -errno (positive TPM return codes are masked to -EIO) > > */ > > int tpm1_get_random(struct tpm_chip *chip, u8 *dest, size_t max) > > { > > @@ -531,8 +531,11 @@ int tpm1_get_random(struct tpm_chip *chip, u8 *dest, size_t max) > > > > rc = tpm_transmit_cmd(chip, &buf, sizeof(out->rng_data_len), > > "attempting get random"); > > - if (rc) > > + if (rc) { > > + if (rc > 0) > > + rc = -EIO; > > goto out; > > + } > > > > out = (struct tpm1_get_random_out *)&buf.data[TPM_HEADER_SIZE]; > > > > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c > > index e74c5b7b64bf..8ffa6af61580 100644 > > --- a/drivers/char/tpm/tpm2-cmd.c > > +++ b/drivers/char/tpm/tpm2-cmd.c > > @@ -301,7 +301,7 @@ struct tpm2_get_random_out { > > * > > * Return: > > * size of the buffer on success, > > - * -errno otherwise > > + * -errno otherwise ((positive TPM return codes are masked to -EIO) > > */ > > int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max) > > { > > @@ -328,8 +328,11 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max) > > offsetof(struct tpm2_get_random_out, > > buffer), > > "attempting get random"); > > - if (err) > > + if (err) { > > + if (err > 0) > > + err = -EIO; > > goto out; > > + } > > > > out = (struct tpm2_get_random_out *) > > &buf.data[TPM_HEADER_SIZE]; > > -- > > 2.17.1 > > > > > > -- > > Kees Cook > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> Applied to my master branch. Jason, Tomas, do you want me to add reviewed-by's? /Jarkko
> On Tue, Apr 02, 2019 at 02:46:25AM +0300, Jarkko Sakkinen wrote: > > On Mon, Apr 01, 2019 at 12:06:07PM -0700, Kees Cook wrote: > > > A "get random" may fail with a TPM error, but those codes were > > > returned as-is to the caller, which assumed the result was the > > > number of bytes that had been written to the target buffer, which > > > could lead to a kernel heap memory exposure and over-read. > > > > > > This fixes tpm1_get_random() to mask positive TPM errors into -EIO, > > > as before. > > > > > > [ 18.092103] tpm tpm0: A TPM error (379) occurred attempting get > random > > > [ 18.092106] usercopy: Kernel memory exposure attempt detected from > SLUB object 'kmalloc-64' (offset 0, size 379)! > > > > > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=1650989 > > > Reported-by: Phil Baker <baker1tex@gmail.com> > > > Reported-by: Craig Robson <craig@zhatt.com> > > > Fixes: 7aee9c52d7ac ("tpm: tpm1: rewrite tpm1_get_random() using > > > tpm_buf structure") > > > Cc: Laura Abbott <labbott@redhat.com> > > > Cc: Tomas Winkler <tomas.winkler@intel.com> > > > Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > > Cc: stable@vger.kernel.org > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > > --- > > > v3: fix never-succeed, limit checks to tpm cmd return (James, Jason) > > > v2: also fix tpm2 implementation (Jason Gunthorpe) > > > --- > > > drivers/char/tpm/tpm1-cmd.c | 7 +++++-- > > > drivers/char/tpm/tpm2-cmd.c | 7 +++++-- > > > 2 files changed, 10 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/char/tpm/tpm1-cmd.c > > > b/drivers/char/tpm/tpm1-cmd.c index 85dcf2654d11..faacbe1ffa1a > > > 100644 > > > --- a/drivers/char/tpm/tpm1-cmd.c > > > +++ b/drivers/char/tpm/tpm1-cmd.c > > > @@ -510,7 +510,7 @@ struct tpm1_get_random_out { > > > * > > > * Return: > > > * * number of bytes read > > > - * * -errno or a TPM return code otherwise > > > + * * -errno (positive TPM return codes are masked to -EIO) > > > */ > > > int tpm1_get_random(struct tpm_chip *chip, u8 *dest, size_t max) { > > > @@ -531,8 +531,11 @@ int tpm1_get_random(struct tpm_chip *chip, u8 > > > *dest, size_t max) > > > > > > rc = tpm_transmit_cmd(chip, &buf, sizeof(out->rng_data_len), > > > "attempting get random"); > > > - if (rc) > > > + if (rc) { > > > + if (rc > 0) > > > + rc = -EIO; > > > goto out; > > > + } > > > > > > out = (struct tpm1_get_random_out > *)&buf.data[TPM_HEADER_SIZE]; > > > > > > diff --git a/drivers/char/tpm/tpm2-cmd.c > > > b/drivers/char/tpm/tpm2-cmd.c index e74c5b7b64bf..8ffa6af61580 > > > 100644 > > > --- a/drivers/char/tpm/tpm2-cmd.c > > > +++ b/drivers/char/tpm/tpm2-cmd.c > > > @@ -301,7 +301,7 @@ struct tpm2_get_random_out { > > > * > > > * Return: > > > * size of the buffer on success, > > > - * -errno otherwise > > > + * -errno otherwise ((positive TPM return codes are masked to -EIO) > > > */ > > > int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max) { > > > @@ -328,8 +328,11 @@ int tpm2_get_random(struct tpm_chip *chip, u8 > *dest, size_t max) > > > offsetof(struct tpm2_get_random_out, > > > buffer), > > > "attempting get random"); > > > - if (err) > > > + if (err) { > > > + if (err > 0) > > > + err = -EIO; > > > goto out; > > > + } > > > > > > out = (struct tpm2_get_random_out *) > > > &buf.data[TPM_HEADER_SIZE]; > > > -- > > > 2.17.1 > > > > > > > > > -- > > > Kees Cook > > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > Applied to my master branch. Jason, Tomas, do you want me to add reviewed- > by's? Sure, it fixes my patch. > /Jarkko
On Tue, Apr 02, 2019 at 07:13:52PM +0000, Winkler, Tomas wrote: > > > > On Tue, Apr 02, 2019 at 02:46:25AM +0300, Jarkko Sakkinen wrote: > > > On Mon, Apr 01, 2019 at 12:06:07PM -0700, Kees Cook wrote: > > > > A "get random" may fail with a TPM error, but those codes were > > > > returned as-is to the caller, which assumed the result was the > > > > number of bytes that had been written to the target buffer, which > > > > could lead to a kernel heap memory exposure and over-read. > > > > > > > > This fixes tpm1_get_random() to mask positive TPM errors into -EIO, > > > > as before. > > > > > > > > [ 18.092103] tpm tpm0: A TPM error (379) occurred attempting get > > random > > > > [ 18.092106] usercopy: Kernel memory exposure attempt detected from > > SLUB object 'kmalloc-64' (offset 0, size 379)! > > > > > > > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=1650989 > > > > Reported-by: Phil Baker <baker1tex@gmail.com> > > > > Reported-by: Craig Robson <craig@zhatt.com> > > > > Fixes: 7aee9c52d7ac ("tpm: tpm1: rewrite tpm1_get_random() using > > > > tpm_buf structure") > > > > Cc: Laura Abbott <labbott@redhat.com> > > > > Cc: Tomas Winkler <tomas.winkler@intel.com> > > > > Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > > > Cc: stable@vger.kernel.org > > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > > > --- > > > > v3: fix never-succeed, limit checks to tpm cmd return (James, Jason) > > > > v2: also fix tpm2 implementation (Jason Gunthorpe) > > > > --- > > > > drivers/char/tpm/tpm1-cmd.c | 7 +++++-- > > > > drivers/char/tpm/tpm2-cmd.c | 7 +++++-- > > > > 2 files changed, 10 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/char/tpm/tpm1-cmd.c > > > > b/drivers/char/tpm/tpm1-cmd.c index 85dcf2654d11..faacbe1ffa1a > > > > 100644 > > > > --- a/drivers/char/tpm/tpm1-cmd.c > > > > +++ b/drivers/char/tpm/tpm1-cmd.c > > > > @@ -510,7 +510,7 @@ struct tpm1_get_random_out { > > > > * > > > > * Return: > > > > * * number of bytes read > > > > - * * -errno or a TPM return code otherwise > > > > + * * -errno (positive TPM return codes are masked to -EIO) > > > > */ > > > > int tpm1_get_random(struct tpm_chip *chip, u8 *dest, size_t max) { > > > > @@ -531,8 +531,11 @@ int tpm1_get_random(struct tpm_chip *chip, u8 > > > > *dest, size_t max) > > > > > > > > rc = tpm_transmit_cmd(chip, &buf, sizeof(out->rng_data_len), > > > > "attempting get random"); > > > > - if (rc) > > > > + if (rc) { > > > > + if (rc > 0) > > > > + rc = -EIO; > > > > goto out; > > > > + } > > > > > > > > out = (struct tpm1_get_random_out > > *)&buf.data[TPM_HEADER_SIZE]; > > > > > > > > diff --git a/drivers/char/tpm/tpm2-cmd.c > > > > b/drivers/char/tpm/tpm2-cmd.c index e74c5b7b64bf..8ffa6af61580 > > > > 100644 > > > > --- a/drivers/char/tpm/tpm2-cmd.c > > > > +++ b/drivers/char/tpm/tpm2-cmd.c > > > > @@ -301,7 +301,7 @@ struct tpm2_get_random_out { > > > > * > > > > * Return: > > > > * size of the buffer on success, > > > > - * -errno otherwise > > > > + * -errno otherwise ((positive TPM return codes are masked to -EIO) > > > > */ > > > > int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max) { > > > > @@ -328,8 +328,11 @@ int tpm2_get_random(struct tpm_chip *chip, u8 > > *dest, size_t max) > > > > offsetof(struct tpm2_get_random_out, > > > > buffer), > > > > "attempting get random"); > > > > - if (err) > > > > + if (err) { > > > > + if (err > 0) > > > > + err = -EIO; > > > > goto out; > > > > + } > > > > > > > > out = (struct tpm2_get_random_out *) > > > > &buf.data[TPM_HEADER_SIZE]; > > > > -- > > > > 2.17.1 > > > > > > > > > > > > -- > > > > Kees Cook > > > > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > > > Applied to my master branch. Jason, Tomas, do you want me to add reviewed- > > by's? > Sure, it fixes my patch. Great, I'll add it. Thank you. Just want to be explicit with these things as I consider them as if I was asking a signature from someone :-) /Jarkko
On 4/3/19 1:52 PM, Jarkko Sakkinen wrote: > On Tue, Apr 02, 2019 at 07:13:52PM +0000, Winkler, Tomas wrote: >> >> >>> On Tue, Apr 02, 2019 at 02:46:25AM +0300, Jarkko Sakkinen wrote: >>>> On Mon, Apr 01, 2019 at 12:06:07PM -0700, Kees Cook wrote: >>>>> A "get random" may fail with a TPM error, but those codes were >>>>> returned as-is to the caller, which assumed the result was the >>>>> number of bytes that had been written to the target buffer, which >>>>> could lead to a kernel heap memory exposure and over-read. >>>>> >>>>> This fixes tpm1_get_random() to mask positive TPM errors into -EIO, >>>>> as before. >>>>> >>>>> [ 18.092103] tpm tpm0: A TPM error (379) occurred attempting get >>> random >>>>> [ 18.092106] usercopy: Kernel memory exposure attempt detected from >>> SLUB object 'kmalloc-64' (offset 0, size 379)! >>>>> >>>>> Link: https://bugzilla.redhat.com/show_bug.cgi?id=1650989 >>>>> Reported-by: Phil Baker <baker1tex@gmail.com> >>>>> Reported-by: Craig Robson <craig@zhatt.com> >>>>> Fixes: 7aee9c52d7ac ("tpm: tpm1: rewrite tpm1_get_random() using >>>>> tpm_buf structure") >>>>> Cc: Laura Abbott <labbott@redhat.com> >>>>> Cc: Tomas Winkler <tomas.winkler@intel.com> >>>>> Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> >>>>> Cc: stable@vger.kernel.org >>>>> Signed-off-by: Kees Cook <keescook@chromium.org> >>>>> --- >>>>> v3: fix never-succeed, limit checks to tpm cmd return (James, Jason) >>>>> v2: also fix tpm2 implementation (Jason Gunthorpe) >>>>> --- >>>>> drivers/char/tpm/tpm1-cmd.c | 7 +++++-- >>>>> drivers/char/tpm/tpm2-cmd.c | 7 +++++-- >>>>> 2 files changed, 10 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/drivers/char/tpm/tpm1-cmd.c >>>>> b/drivers/char/tpm/tpm1-cmd.c index 85dcf2654d11..faacbe1ffa1a >>>>> 100644 >>>>> --- a/drivers/char/tpm/tpm1-cmd.c >>>>> +++ b/drivers/char/tpm/tpm1-cmd.c >>>>> @@ -510,7 +510,7 @@ struct tpm1_get_random_out { >>>>> * >>>>> * Return: >>>>> * * number of bytes read >>>>> - * * -errno or a TPM return code otherwise >>>>> + * * -errno (positive TPM return codes are masked to -EIO) >>>>> */ >>>>> int tpm1_get_random(struct tpm_chip *chip, u8 *dest, size_t max) { >>>>> @@ -531,8 +531,11 @@ int tpm1_get_random(struct tpm_chip *chip, u8 >>>>> *dest, size_t max) >>>>> >>>>> rc = tpm_transmit_cmd(chip, &buf, sizeof(out->rng_data_len), >>>>> "attempting get random"); >>>>> - if (rc) >>>>> + if (rc) { >>>>> + if (rc > 0) >>>>> + rc = -EIO; >>>>> goto out; >>>>> + } >>>>> >>>>> out = (struct tpm1_get_random_out >>> *)&buf.data[TPM_HEADER_SIZE]; >>>>> >>>>> diff --git a/drivers/char/tpm/tpm2-cmd.c >>>>> b/drivers/char/tpm/tpm2-cmd.c index e74c5b7b64bf..8ffa6af61580 >>>>> 100644 >>>>> --- a/drivers/char/tpm/tpm2-cmd.c >>>>> +++ b/drivers/char/tpm/tpm2-cmd.c >>>>> @@ -301,7 +301,7 @@ struct tpm2_get_random_out { >>>>> * >>>>> * Return: >>>>> * size of the buffer on success, >>>>> - * -errno otherwise >>>>> + * -errno otherwise ((positive TPM return codes are masked to -EIO) >>>>> */ >>>>> int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max) { >>>>> @@ -328,8 +328,11 @@ int tpm2_get_random(struct tpm_chip *chip, u8 >>> *dest, size_t max) >>>>> offsetof(struct tpm2_get_random_out, >>>>> buffer), >>>>> "attempting get random"); >>>>> - if (err) >>>>> + if (err) { >>>>> + if (err > 0) >>>>> + err = -EIO; >>>>> goto out; >>>>> + } >>>>> >>>>> out = (struct tpm2_get_random_out *) >>>>> &buf.data[TPM_HEADER_SIZE]; >>>>> -- >>>>> 2.17.1 >>>>> >>>>> >>>>> -- >>>>> Kees Cook >>>> >>>> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> >>> >>> Applied to my master branch. Jason, Tomas, do you want me to add reviewed- >>> by's? >> Sure, it fixes my patch. > > Great, I'll add it. Thank you. Just want to be explicit with these > things as I consider them as if I was asking a signature from someone > :-) > > /Jarkko > Was this intended to go in for 5.2? I still don't see it in the tree. Thanks, Laura
On Tue, May 28, 2019 at 03:02:49PM -0400, Laura Abbott wrote: > > Great, I'll add it. Thank you. Just want to be explicit with these > > things as I consider them as if I was asking a signature from someone > > :-) > > > > /Jarkko > > > Was this intended to go in for 5.2? I still don't see it in the tree. Was intended but I failed to notice that I should start to send PRs to Linus instead of security tree and was waiting for security tree to be rebased. I'll include to the next PR. /Jarkko
diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c index 85dcf2654d11..faacbe1ffa1a 100644 --- a/drivers/char/tpm/tpm1-cmd.c +++ b/drivers/char/tpm/tpm1-cmd.c @@ -510,7 +510,7 @@ struct tpm1_get_random_out { * * Return: * * number of bytes read - * * -errno or a TPM return code otherwise + * * -errno (positive TPM return codes are masked to -EIO) */ int tpm1_get_random(struct tpm_chip *chip, u8 *dest, size_t max) { @@ -531,8 +531,11 @@ int tpm1_get_random(struct tpm_chip *chip, u8 *dest, size_t max) rc = tpm_transmit_cmd(chip, &buf, sizeof(out->rng_data_len), "attempting get random"); - if (rc) + if (rc) { + if (rc > 0) + rc = -EIO; goto out; + } out = (struct tpm1_get_random_out *)&buf.data[TPM_HEADER_SIZE]; diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c index e74c5b7b64bf..8ffa6af61580 100644 --- a/drivers/char/tpm/tpm2-cmd.c +++ b/drivers/char/tpm/tpm2-cmd.c @@ -301,7 +301,7 @@ struct tpm2_get_random_out { * * Return: * size of the buffer on success, - * -errno otherwise + * -errno otherwise ((positive TPM return codes are masked to -EIO) */ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max) { @@ -328,8 +328,11 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max) offsetof(struct tpm2_get_random_out, buffer), "attempting get random"); - if (err) + if (err) { + if (err > 0) + err = -EIO; goto out; + } out = (struct tpm2_get_random_out *) &buf.data[TPM_HEADER_SIZE];
A "get random" may fail with a TPM error, but those codes were returned as-is to the caller, which assumed the result was the number of bytes that had been written to the target buffer, which could lead to a kernel heap memory exposure and over-read. This fixes tpm1_get_random() to mask positive TPM errors into -EIO, as before. [ 18.092103] tpm tpm0: A TPM error (379) occurred attempting get random [ 18.092106] usercopy: Kernel memory exposure attempt detected from SLUB object 'kmalloc-64' (offset 0, size 379)! Link: https://bugzilla.redhat.com/show_bug.cgi?id=1650989 Reported-by: Phil Baker <baker1tex@gmail.com> Reported-by: Craig Robson <craig@zhatt.com> Fixes: 7aee9c52d7ac ("tpm: tpm1: rewrite tpm1_get_random() using tpm_buf structure") Cc: Laura Abbott <labbott@redhat.com> Cc: Tomas Winkler <tomas.winkler@intel.com> Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> Cc: stable@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- v3: fix never-succeed, limit checks to tpm cmd return (James, Jason) v2: also fix tpm2 implementation (Jason Gunthorpe) --- drivers/char/tpm/tpm1-cmd.c | 7 +++++-- drivers/char/tpm/tpm2-cmd.c | 7 +++++-- 2 files changed, 10 insertions(+), 4 deletions(-)