| Message ID | 1465610107-87762-4-git-send-email-eswierk@skyportsystems.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jun 10, 2016 at 06:55:05PM -0700, Ed Swierk wrote: > Factor sending the TPM_GetCapability command and validating the result > from tpm_get_timeouts() into a new function. Return all errors to the > caller rather than swallowing them (e.g. when tpm_transmit_cmd() > returns nonzero). LGTM but I have to test this on next week. I'll give final Reviewed/Tested-by after that. Thanks for the good work. > Signed-off-by: Ed Swierk <eswierk@skyportsystems.com> > --- > drivers/char/tpm/tpm-interface.c | 96 ++++++++++++++++++++++------------------ > 1 file changed, 52 insertions(+), 44 deletions(-) /Jarkko ------------------------------------------------------------------------------ What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic patterns at an interface-level. Reveals which users, apps, and protocols are consuming the most bandwidth. Provides multi-vendor support for NetFlow, J-Flow, sFlow and other flows. Make informed decisions using capacity planning reports. http://sdm.link/zohomanageengine
On Fri, Jun 10, 2016 at 06:55:05PM -0700, Ed Swierk wrote: > Factor sending the TPM_GetCapability command and validating the result > from tpm_get_timeouts() into a new function. Return all errors to the > caller rather than swallowing them (e.g. when tpm_transmit_cmd() > returns nonzero). > > Signed-off-by: Ed Swierk <eswierk@skyportsystems.com> > --- > drivers/char/tpm/tpm-interface.c | 96 ++++++++++++++++++++++------------------ > 1 file changed, 52 insertions(+), 44 deletions(-) I'm sorry but just now that I started applying these patches this patch started to bother me. > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c > index cc1e5bc..4d1f62c 100644 > --- a/drivers/char/tpm/tpm-interface.c > +++ b/drivers/char/tpm/tpm-interface.c > @@ -502,6 +502,52 @@ static int tpm_startup(struct tpm_chip *chip, __be16 startup_type) > "attempting to start the TPM"); > } > > +static int tpm_get_cap_prop(struct tpm_chip *chip, __be32 type, int size, > + cap_t *cap, char *desc) > +{ > + struct tpm_cmd_t tpm_cmd; > + ssize_t rc; > + > + tpm_cmd.header.in = tpm_getcap_header; > + tpm_cmd.params.getcap_in.cap = TPM_CAP_PROP; > + tpm_cmd.params.getcap_in.subcap_size = cpu_to_be32(4); > + tpm_cmd.params.getcap_in.subcap = type; > + rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, NULL); > + > + if (rc == TPM_ERR_INVALID_POSTINIT) { > + /* The TPM is not started, we are the first to talk to it. > + Execute a startup command. */ > + dev_info(chip->pdev, "Issuing TPM_STARTUP\n"); > + if (tpm_startup(chip, TPM_ST_CLEAR)) > + return rc; > + > + tpm_cmd.header.in = tpm_getcap_header; > + tpm_cmd.params.getcap_in.cap = TPM_CAP_PROP; > + tpm_cmd.params.getcap_in.subcap_size = cpu_to_be32(4); > + tpm_cmd.params.getcap_in.subcap = type; > + rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, > + NULL); > + } I think inside tpm_get_timeouts() I'd rather something along the lines (with error handling and such details taken away): rc = tpm_getcap(...); if (rc == TPM_ERR_INVALID_POSTINIT) { tpm_startup(...); tpm_getca(...); } > + if (rc) { > + dev_err(chip->pdev, > + "Error %zd reading %s\n", rc, desc); > + return rc; > + } > + > + if (be32_to_cpu(tpm_cmd.header.out.return_code) != 0 || > + be32_to_cpu(tpm_cmd.header.out.length) > + != sizeof(tpm_cmd.header.out) + sizeof(u32) + size * sizeof(u32)) { > + dev_err(chip->pdev, > + "Bad return code or length reading %s\n", desc); > + return -EINVAL; > + } This is bogus code. All this kind of checks should be contained in tpm_transmit_cmd(). This is easily "fixed" by moving tpm_getcap() :) /Jarkko ------------------------------------------------------------------------------ What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic patterns at an interface-level. Reveals which users, apps, and protocols are consuming the most bandwidth. Provides multi-vendor support for NetFlow, J-Flow, sFlow and other flows. Make informed decisions using capacity planning reports. http://sdm.link/zohomanageengine
On Sun, Jun 19, 2016 at 5:12 AM, Jarkko Sakkinen < jarkko.sakkinen@linux.intel.com> wrote: > I think inside tpm_get_timeouts() I'd rather something along the lines > (with error handling and such details taken away): > > rc = tpm_getcap(...); > > if (rc == TPM_ERR_INVALID_POSTINIT) { > tpm_startup(...); > tpm_getca(...); > } > Agreed. > + if (rc) { > > + dev_err(chip->pdev, > > + "Error %zd reading %s\n", rc, desc); > > + return rc; > > + } > > + > > + if (be32_to_cpu(tpm_cmd.header.out.return_code) != 0 || > > + be32_to_cpu(tpm_cmd.header.out.length) > > + != sizeof(tpm_cmd.header.out) + sizeof(u32) + size * > sizeof(u32)) { > > + dev_err(chip->pdev, > > + "Bad return code or length reading %s\n", desc); > > + return -EINVAL; > > + } > > This is bogus code. All this kind of checks should be contained in > tpm_transmit_cmd(). This is easily "fixed" by moving tpm_getcap() :) > tpm_transmit_cmd() already checks the return_code, so that's easy. It doesn't know the expected length, but tpm_getcap() does (or can, based on the subcap_id). --Ed ------------------------------------------------------------------------------ Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San Francisco, CA to explore cutting-edge tech and listen to tech luminaries present their vision of the future. This family event has something for everyone, including kids. Get more information and register today. http://sdm.link/attshape
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c index cc1e5bc..4d1f62c 100644 --- a/drivers/char/tpm/tpm-interface.c +++ b/drivers/char/tpm/tpm-interface.c @@ -502,6 +502,52 @@ static int tpm_startup(struct tpm_chip *chip, __be16 startup_type) "attempting to start the TPM"); } +static int tpm_get_cap_prop(struct tpm_chip *chip, __be32 type, int size, + cap_t *cap, char *desc) +{ + struct tpm_cmd_t tpm_cmd; + ssize_t rc; + + tpm_cmd.header.in = tpm_getcap_header; + tpm_cmd.params.getcap_in.cap = TPM_CAP_PROP; + tpm_cmd.params.getcap_in.subcap_size = cpu_to_be32(4); + tpm_cmd.params.getcap_in.subcap = type; + rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, NULL); + + if (rc == TPM_ERR_INVALID_POSTINIT) { + /* The TPM is not started, we are the first to talk to it. + Execute a startup command. */ + dev_info(chip->pdev, "Issuing TPM_STARTUP\n"); + if (tpm_startup(chip, TPM_ST_CLEAR)) + return rc; + + tpm_cmd.header.in = tpm_getcap_header; + tpm_cmd.params.getcap_in.cap = TPM_CAP_PROP; + tpm_cmd.params.getcap_in.subcap_size = cpu_to_be32(4); + tpm_cmd.params.getcap_in.subcap = type; + rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, + NULL); + } + + if (rc) { + dev_err(chip->pdev, + "Error %zd reading %s\n", rc, desc); + return rc; + } + + if (be32_to_cpu(tpm_cmd.header.out.return_code) != 0 || + be32_to_cpu(tpm_cmd.header.out.length) + != sizeof(tpm_cmd.header.out) + sizeof(u32) + size * sizeof(u32)) { + dev_err(chip->pdev, + "Bad return code or length reading %s\n", desc); + return -EINVAL; + } + + memcpy(cap, &tpm_cmd.params.getcap_out.cap, sizeof(cap_t)); + + return 0; +} + int tpm_get_timeouts(struct tpm_chip *chip) { struct tpm_cmd_t tpm_cmd; @@ -510,37 +556,10 @@ int tpm_get_timeouts(struct tpm_chip *chip) struct duration_t *duration_cap; ssize_t rc; - tpm_cmd.header.in = tpm_getcap_header; - tpm_cmd.params.getcap_in.cap = TPM_CAP_PROP; - tpm_cmd.params.getcap_in.subcap_size = cpu_to_be32(4); - tpm_cmd.params.getcap_in.subcap = TPM_CAP_PROP_TIS_TIMEOUT; - rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, NULL); - - if (rc == TPM_ERR_INVALID_POSTINIT) { - /* The TPM is not started, we are the first to talk to it. - Execute a startup command. */ - dev_info(chip->pdev, "Issuing TPM_STARTUP"); - if (tpm_startup(chip, TPM_ST_CLEAR)) - return rc; - - tpm_cmd.header.in = tpm_getcap_header; - tpm_cmd.params.getcap_in.cap = TPM_CAP_PROP; - tpm_cmd.params.getcap_in.subcap_size = cpu_to_be32(4); - tpm_cmd.params.getcap_in.subcap = TPM_CAP_PROP_TIS_TIMEOUT; - rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, - NULL); - } - if (rc) { - dev_err(chip->pdev, - "A TPM error (%zd) occurred attempting to determine the timeouts\n", - rc); - goto duration; - } - - if (be32_to_cpu(tpm_cmd.header.out.return_code) != 0 || - be32_to_cpu(tpm_cmd.header.out.length) - != sizeof(tpm_cmd.header.out) + sizeof(u32) + 4 * sizeof(u32)) - return -EINVAL; + rc = tpm_get_cap_prop(chip, TPM_CAP_PROP_TIS_TIMEOUT, 4, + &tpm_cmd.params.getcap_out.cap, "timeouts"); + if (rc) + return rc; old_timeout[0] = be32_to_cpu(tpm_cmd.params.getcap_out.cap.timeout.a); old_timeout[1] = be32_to_cpu(tpm_cmd.params.getcap_out.cap.timeout.b); @@ -583,22 +602,11 @@ int tpm_get_timeouts(struct tpm_chip *chip) chip->vendor.timeout_c = usecs_to_jiffies(new_timeout[2]); chip->vendor.timeout_d = usecs_to_jiffies(new_timeout[3]); -duration: - tpm_cmd.header.in = tpm_getcap_header; - tpm_cmd.params.getcap_in.cap = TPM_CAP_PROP; - tpm_cmd.params.getcap_in.subcap_size = cpu_to_be32(4); - tpm_cmd.params.getcap_in.subcap = TPM_CAP_PROP_TIS_DURATION; - - rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, - "attempting to determine the durations"); + rc = tpm_get_cap_prop(chip, TPM_CAP_PROP_TIS_DURATION, 3, + &tpm_cmd.params.getcap_out.cap, "durations"); if (rc) return rc; - if (be32_to_cpu(tpm_cmd.header.out.return_code) != 0 || - be32_to_cpu(tpm_cmd.header.out.length) - != sizeof(tpm_cmd.header.out) + sizeof(u32) + 3 * sizeof(u32)) - return -EINVAL; - duration_cap = &tpm_cmd.params.getcap_out.cap.duration; chip->vendor.duration[TPM_SHORT] = usecs_to_jiffies(be32_to_cpu(duration_cap->tpm_short));
Factor sending the TPM_GetCapability command and validating the result from tpm_get_timeouts() into a new function. Return all errors to the caller rather than swallowing them (e.g. when tpm_transmit_cmd() returns nonzero). Signed-off-by: Ed Swierk <eswierk@skyportsystems.com> --- drivers/char/tpm/tpm-interface.c | 96 ++++++++++++++++++++++------------------ 1 file changed, 52 insertions(+), 44 deletions(-)