[PATCH] Check for error return values from get_burstcount.

[PATCH] Check for error return values from get_burstcount.

[Josh Zimmerman]

If the TPM we're connecting to uses a static burst count, it will report
a burst count of zero throughout the response read. However, get_burstcount
assumes that a response of zero indicates that the TPM is not ready to
receive more data. In this case, it returns a negative error code, which
is passed on to tpm_tis_{write,read}_bytes as a u16, causing
them to read/write far too many bytes.

This patch checks for negative return codes and bails out from recv_data
and tpm_tis_send_data.
---
 drivers/char/tpm/tpm_tis_core.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index e3bf31b..d0301dc 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -186,6 +186,12 @@ static int recv_data(struct tpm_chip *chip, u8 *buf, size_t count)
 				 chip->timeout_c,
 				 &priv->read_queue, true) == 0) {
 		burstcnt = min_t(int, get_burstcount(chip), count - size);
+		if (burstcnt < 0) {
+			dev_err(&chip->dev,
+				"Unable to read burstcount in %s:%d (%s)\n",
+				__FILE__, __LINE__, __func__);
+			return rc;
+		}
 
 		rc = tpm_tis_read_bytes(priv, TPM_DATA_FIFO(priv->locality),
 					burstcnt, buf + size);
@@ -272,6 +278,13 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
 
 	while (count < len - 1) {
 		burstcnt = min_t(int, get_burstcount(chip), len - count - 1);
+		if (burstcnt < 0) {
+			dev_err(&chip->dev,
+				"Unable to read burstcount in %s:%d (%s)\n",
+				__FILE__, __LINE__, __func__);
+			rc = burstcnt;
+			goto out_err;
+		}
 		rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality),
 					 burstcnt, buf + count);
 		if (rc < 0)
-- 
2.8.0.rc3.226.g39d4020


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

Re: [PATCH] Check for error return values from get_burstcount.

[Jarkko Sakkinen]

On Thu, Oct 20, 2016 at 05:21:29PM -0700, Josh Zimmerman wrote:
> If the TPM we're connecting to uses a static burst count, it will report
> a burst count of zero throughout the response read. However, get_burstcount
> assumes that a response of zero indicates that the TPM is not ready to
> receive more data. In this case, it returns a negative error code, which
> is passed on to tpm_tis_{write,read}_bytes as a u16, causing
> them to read/write far too many bytes.
> 
> This patch checks for negative return codes and bails out from recv_data
> and tpm_tis_send_data.

I guess this would need a "Fixes:" tag, wouldn't it? I would also add

Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

> ---
>  drivers/char/tpm/tpm_tis_core.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index e3bf31b..d0301dc 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -186,6 +186,12 @@ static int recv_data(struct tpm_chip *chip, u8 *buf, size_t count)
>  				 chip->timeout_c,
>  				 &priv->read_queue, true) == 0) {
>  		burstcnt = min_t(int, get_burstcount(chip), count - size);
> +		if (burstcnt < 0) {
> +			dev_err(&chip->dev,
> +				"Unable to read burstcount in %s:%d (%s)\n",
> +				__FILE__, __LINE__, __func__);
> +			return rc;
> +		}

"return burstcnt;"

I guess __func__ would be enough to deduce the call site?

>  		rc = tpm_tis_read_bytes(priv, TPM_DATA_FIFO(priv->locality),
>  					burstcnt, buf + size);
> @@ -272,6 +278,13 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
>  
>  	while (count < len - 1) {
>  		burstcnt = min_t(int, get_burstcount(chip), len - count - 1);
> +		if (burstcnt < 0) {
> +			dev_err(&chip->dev,
> +				"Unable to read burstcount in %s:%d (%s)\n",
> +				__FILE__, __LINE__, __func__);
> +			rc = burstcnt;
> +			goto out_err;
> +		}
>  		rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality),
>  					 burstcnt, buf + count);
>  		if (rc < 0)
> -- 
> 2.8.0.rc3.226.g39d4020

/Jarkko

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

Re: [PATCH] Check for error return values from get_burstcount.

[Josh Zimmerman]

On Fri, Oct 21, 2016 at 9:13 AM, Jarkko Sakkinen
<jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
> On Thu, Oct 20, 2016 at 05:21:29PM -0700, Josh Zimmerman wrote:
>> If the TPM we're connecting to uses a static burst count, it will report
>> a burst count of zero throughout the response read. However, get_burstcount
>> assumes that a response of zero indicates that the TPM is not ready to
>> receive more data. In this case, it returns a negative error code, which
>> is passed on to tpm_tis_{write,read}_bytes as a u16, causing
>> them to read/write far too many bytes.
>>
>> This patch checks for negative return codes and bails out from recv_data
>> and tpm_tis_send_data.
>
> I guess this would need a "Fixes:" tag, wouldn't it?
Ah, yes. Good point.

>  I would also add
>
> Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Will do when I mail the updated patch.

>> ---
>>  drivers/char/tpm/tpm_tis_core.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
>> index e3bf31b..d0301dc 100644
>> --- a/drivers/char/tpm/tpm_tis_core.c
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>> @@ -186,6 +186,12 @@ static int recv_data(struct tpm_chip *chip, u8 *buf, size_t count)
>>                                chip->timeout_c,
>>                                &priv->read_queue, true) == 0) {
>>               burstcnt = min_t(int, get_burstcount(chip), count - size);
>> +             if (burstcnt < 0) {
>> +                     dev_err(&chip->dev,
>> +                             "Unable to read burstcount in %s:%d (%s)\n",
>> +                             __FILE__, __LINE__, __func__);
>> +                     return rc;
>> +             }
>
> "return burstcnt;"
Done, thanks!

> I guess __func__ would be enough to deduce the call site?
It is not; would you suggest removing it since it's redundant with
__LINE__ and __FILE__? Or is there some other macro you know of that
could be useful for determining the call site? (I had included it
initially for the sake of ease of log reading, but that's perhaps not
very compelling.)

>
>>               rc = tpm_tis_read_bytes(priv, TPM_DATA_FIFO(priv->locality),
>>                                       burstcnt, buf + size);
>> @@ -272,6 +278,13 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
>>
>>       while (count < len - 1) {
>>               burstcnt = min_t(int, get_burstcount(chip), len - count - 1);
>> +             if (burstcnt < 0) {
>> +                     dev_err(&chip->dev,
>> +                             "Unable to read burstcount in %s:%d (%s)\n",
>> +                             __FILE__, __LINE__, __func__);
>> +                     rc = burstcnt;
>> +                     goto out_err;
>> +             }
>>               rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality),
>>                                        burstcnt, buf + count);
>>               if (rc < 0)
>> --
>> 2.8.0.rc3.226.g39d4020
>
> /Jarkko

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

Re: [PATCH] Check for error return values from get_burstcount.

[Jarkko Sakkinen]

On Fri, Oct 21, 2016 at 04:01:13PM -0700, Josh Zimmerman wrote:
> On Fri, Oct 21, 2016 at 9:13 AM, Jarkko Sakkinen
> <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
> > On Thu, Oct 20, 2016 at 05:21:29PM -0700, Josh Zimmerman wrote:
> >> If the TPM we're connecting to uses a static burst count, it will report
> >> a burst count of zero throughout the response read. However, get_burstcount
> >> assumes that a response of zero indicates that the TPM is not ready to
> >> receive more data. In this case, it returns a negative error code, which
> >> is passed on to tpm_tis_{write,read}_bytes as a u16, causing
> >> them to read/write far too many bytes.
> >>
> >> This patch checks for negative return codes and bails out from recv_data
> >> and tpm_tis_send_data.
> >
> > I guess this would need a "Fixes:" tag, wouldn't it?
> Ah, yes. Good point.
> 
> >  I would also add
> >
> > Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Will do when I mail the updated patch.
> 
> >> ---
> >>  drivers/char/tpm/tpm_tis_core.c | 13 +++++++++++++
> >>  1 file changed, 13 insertions(+)
> >>
> >> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> >> index e3bf31b..d0301dc 100644
> >> --- a/drivers/char/tpm/tpm_tis_core.c
> >> +++ b/drivers/char/tpm/tpm_tis_core.c
> >> @@ -186,6 +186,12 @@ static int recv_data(struct tpm_chip *chip, u8 *buf, size_t count)
> >>                                chip->timeout_c,
> >>                                &priv->read_queue, true) == 0) {
> >>               burstcnt = min_t(int, get_burstcount(chip), count - size);
> >> +             if (burstcnt < 0) {
> >> +                     dev_err(&chip->dev,
> >> +                             "Unable to read burstcount in %s:%d (%s)\n",
> >> +                             __FILE__, __LINE__, __func__);
> >> +                     return rc;
> >> +             }
> >
> > "return burstcnt;"
> Done, thanks!
> 
> > I guess __func__ would be enough to deduce the call site?
> It is not; would you suggest removing it since it's redundant with
> __LINE__ and __FILE__? Or is there some other macro you know of that
> could be useful for determining the call site? (I had included it
> initially for the sake of ease of log reading, but that's perhaps not
> very compelling.)

It's an internal function to tpm_tis_core.c and there are exactly
two call sites for it. Just by printing __func__ you can determine
where it fails.

/Jarkko

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

Re: [PATCH] Check for error return values from get_burstcount.

[Josh Zimmerman]

On Sat, Oct 22, 2016 at 5:04 AM, Jarkko Sakkinen
<jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
> On Fri, Oct 21, 2016 at 04:01:13PM -0700, Josh Zimmerman wrote:
>> On Fri, Oct 21, 2016 at 9:13 AM, Jarkko Sakkinen
>> <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
>> > On Thu, Oct 20, 2016 at 05:21:29PM -0700, Josh Zimmerman wrote:
>> >> If the TPM we're connecting to uses a static burst count, it will report
>> >> a burst count of zero throughout the response read. However, get_burstcount
>> >> assumes that a response of zero indicates that the TPM is not ready to
>> >> receive more data. In this case, it returns a negative error code, which
>> >> is passed on to tpm_tis_{write,read}_bytes as a u16, causing
>> >> them to read/write far too many bytes.
>> >>
>> >> This patch checks for negative return codes and bails out from recv_data
>> >> and tpm_tis_send_data.
>> >
>> > I guess this would need a "Fixes:" tag, wouldn't it?
>> Ah, yes. Good point.
>>
>> >  I would also add
>> >
>> > Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Will do when I mail the updated patch.
>>
>> >> ---
>> >>  drivers/char/tpm/tpm_tis_core.c | 13 +++++++++++++
>> >>  1 file changed, 13 insertions(+)
>> >>
>> >> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
>> >> index e3bf31b..d0301dc 100644
>> >> --- a/drivers/char/tpm/tpm_tis_core.c
>> >> +++ b/drivers/char/tpm/tpm_tis_core.c
>> >> @@ -186,6 +186,12 @@ static int recv_data(struct tpm_chip *chip, u8 *buf, size_t count)
>> >>                                chip->timeout_c,
>> >>                                &priv->read_queue, true) == 0) {
>> >>               burstcnt = min_t(int, get_burstcount(chip), count - size);
>> >> +             if (burstcnt < 0) {
>> >> +                     dev_err(&chip->dev,
>> >> +                             "Unable to read burstcount in %s:%d (%s)\n",
>> >> +                             __FILE__, __LINE__, __func__);
>> >> +                     return rc;
>> >> +             }
>> >
>> > "return burstcnt;"
>> Done, thanks!
>>
>> > I guess __func__ would be enough to deduce the call site?
>> It is not; would you suggest removing it since it's redundant with
>> __LINE__ and __FILE__? Or is there some other macro you know of that
>> could be useful for determining the call site? (I had included it
>> initially for the sake of ease of log reading, but that's perhaps not
>> very compelling.)
>
> It's an internal function to tpm_tis_core.c and there are exactly
> two call sites for it. Just by printing __func__ you can determine
> where it fails.

I see; I misunderstood. I thought you were asking about the call site
of the function(s) that call recv_data and tpm_tis_send_data.

I think I'd prefer to leave the line number information in for now, in
case more calls to get_burstcount are added later in either of those
functions.

Josh

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