Re: [PATCH v5 13/17] tpm: use tpm_try_get_ops() in tpm-sysfs.c.

[Stefan Berger <stefanb@linux.ibm.com>]

On 11/8/18 9:15 AM, Jarkko Sakkinen wrote:
> Use tpm_try_get_ops() in tpm-sysfs.c so that we can consider moving
> other decorations (locking, localities, power management for example)
> inside it. This direction can be of course taken only after other call
> sites for tpm_transmit() have been treated in the same way.
>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

> ---
>   drivers/char/tpm/tpm-sysfs.c | 123 ++++++++++++++++++++++-------------
>   1 file changed, 78 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
> index 03e704f99ed6..3733491671ca 100644
> --- a/drivers/char/tpm/tpm-sysfs.c
> +++ b/drivers/char/tpm/tpm-sysfs.c
> @@ -39,7 +39,6 @@ static ssize_t pubek_show(struct device *dev, struct device_attribute *attr,
>   {
>   	struct tpm_buf tpm_buf;
>   	struct tpm_readpubek_out *out;
> -	ssize_t rc;
>   	int i;
>   	char *str = buf;
>   	struct tpm_chip *chip = to_tpm_chip(dev);
> @@ -47,17 +46,17 @@ static ssize_t pubek_show(struct device *dev, struct device_attribute *attr,
>
>   	memset(&anti_replay, 0, sizeof(anti_replay));
>
> -	if (tpm_buf_init(&tpm_buf, TPM_TAG_RQU_COMMAND, TPM_ORD_READPUBEK))
> +	if (tpm_try_get_ops(chip))
>   		return 0;
>
> +	if (tpm_buf_init(&tpm_buf, TPM_TAG_RQU_COMMAND, TPM_ORD_READPUBEK))
> +		goto out_ops;
> +
>   	tpm_buf_append(&tpm_buf, anti_replay, sizeof(anti_replay));
>
> -	rc = tpm_transmit_cmd(chip, &tpm_buf, READ_PUBEK_RESULT_MIN_BODY_SIZE,
> -			      0, "attempting to read the PUBEK");
> -	if (rc) {
> -		tpm_buf_destroy(&tpm_buf);
> -		return 0;
> -	}
> +	if (tpm_transmit_cmd(chip, &tpm_buf, READ_PUBEK_RESULT_MIN_BODY_SIZE,
> +			      0, "attempting to read the PUBEK"))
> +		goto out_buf;
>
>   	out = (struct tpm_readpubek_out *)&tpm_buf.data[10];
>   	str +=
> @@ -88,9 +87,11 @@ static ssize_t pubek_show(struct device *dev, struct device_attribute *attr,
>   			str += sprintf(str, "\n");
>   	}
>
> -	rc = str - buf;
> +out_buf:
>   	tpm_buf_destroy(&tpm_buf);
> -	return rc;
> +out_ops:
> +	tpm_put_ops(chip);
> +	return str - buf;
>   }
>   static DEVICE_ATTR_RO(pubek);
>
> @@ -103,10 +104,15 @@ static ssize_t pcrs_show(struct device *dev, struct device_attribute *attr,
>   	char *str = buf;
>   	struct tpm_chip *chip = to_tpm_chip(dev);
>
> +	if (tpm_try_get_ops(chip))
> +		return 0;
> +
>   	if (tpm1_getcap(chip, TPM_CAP_PROP_PCR, &cap,
>   			"attempting to determine the number of PCRS",
> -			sizeof(cap.num_pcrs)))
> +			sizeof(cap.num_pcrs))) {
> +		tpm_put_ops(chip);
>   		return 0;
> +	}
>
>   	num_pcrs = be32_to_cpu(cap.num_pcrs);
>   	for (i = 0; i < num_pcrs; i++) {
> @@ -119,6 +125,7 @@ static ssize_t pcrs_show(struct device *dev, struct device_attribute *attr,
>   			str += sprintf(str, "%02X ", digest[j]);
>   		str += sprintf(str, "\n");
>   	}
> +	tpm_put_ops(chip);
>   	return str - buf;
>   }
>   static DEVICE_ATTR_RO(pcrs);
> @@ -126,16 +133,21 @@ static DEVICE_ATTR_RO(pcrs);
>   static ssize_t enabled_show(struct device *dev, struct device_attribute *attr,
>   		     char *buf)
>   {
> +	struct tpm_chip *chip = to_tpm_chip(dev);
> +	ssize_t rc = 0;
>   	cap_t cap;
> -	ssize_t rc;
>
> -	rc = tpm1_getcap(to_tpm_chip(dev), TPM_CAP_FLAG_PERM, &cap,
> -			 "attempting to determine the permanent enabled state",
> -			 sizeof(cap.perm_flags));
> -	if (rc)
> +	if (tpm_try_get_ops(chip))
>   		return 0;
>
> +	if (tpm1_getcap(chip, TPM_CAP_FLAG_PERM, &cap,
> +			"attempting to determine the permanent enabled state",
> +			sizeof(cap.perm_flags)))
> +		goto out_ops;
> +
>   	rc = sprintf(buf, "%d\n", !cap.perm_flags.disable);
> +out_ops:
> +	tpm_put_ops(chip);
>   	return rc;
>   }
>   static DEVICE_ATTR_RO(enabled);
> @@ -143,16 +155,21 @@ static DEVICE_ATTR_RO(enabled);
>   static ssize_t active_show(struct device *dev, struct device_attribute *attr,
>   		    char *buf)
>   {
> +	struct tpm_chip *chip = to_tpm_chip(dev);
> +	ssize_t rc = 0;
>   	cap_t cap;
> -	ssize_t rc;
>
> -	rc = tpm1_getcap(to_tpm_chip(dev), TPM_CAP_FLAG_PERM, &cap,
> -			 "attempting to determine the permanent active state",
> -			 sizeof(cap.perm_flags));
> -	if (rc)
> +	if (tpm_try_get_ops(chip))
>   		return 0;
>
> +	if (tpm1_getcap(chip, TPM_CAP_FLAG_PERM, &cap,
> +			"attempting to determine the permanent active state",
> +			sizeof(cap.perm_flags)))
> +		goto out_ops;
> +
>   	rc = sprintf(buf, "%d\n", !cap.perm_flags.deactivated);
> +out_ops:
> +	tpm_put_ops(chip);
>   	return rc;
>   }
>   static DEVICE_ATTR_RO(active);
> @@ -160,16 +177,21 @@ static DEVICE_ATTR_RO(active);
>   static ssize_t owned_show(struct device *dev, struct device_attribute *attr,
>   			  char *buf)
>   {
> +	struct tpm_chip *chip = to_tpm_chip(dev);
> +	ssize_t rc = 0;
>   	cap_t cap;
> -	ssize_t rc;
>
> -	rc = tpm1_getcap(to_tpm_chip(dev), TPM_CAP_PROP_OWNER, &cap,
> -			 "attempting to determine the owner state",
> -			 sizeof(cap.owned));
> -	if (rc)
> +	if (tpm_try_get_ops(chip))
>   		return 0;
>
> +	if (tpm1_getcap(to_tpm_chip(dev), TPM_CAP_PROP_OWNER, &cap,
> +			"attempting to determine the owner state",
> +			sizeof(cap.owned)))
> +		goto out_ops;
> +
>   	rc = sprintf(buf, "%d\n", cap.owned);
> +out_ops:
> +	tpm_put_ops(chip);
>   	return rc;
>   }
>   static DEVICE_ATTR_RO(owned);
> @@ -177,16 +199,21 @@ static DEVICE_ATTR_RO(owned);
>   static ssize_t temp_deactivated_show(struct device *dev,
>   				     struct device_attribute *attr, char *buf)
>   {
> +	struct tpm_chip *chip = to_tpm_chip(dev);
> +	ssize_t rc = 0;
>   	cap_t cap;
> -	ssize_t rc;
>
> -	rc = tpm1_getcap(to_tpm_chip(dev), TPM_CAP_FLAG_VOL, &cap,
> -			 "attempting to determine the temporary state",
> -			 sizeof(cap.stclear_flags));
> -	if (rc)
> +	if (tpm_try_get_ops(chip))
>   		return 0;
>
> +	if (tpm1_getcap(to_tpm_chip(dev), TPM_CAP_FLAG_VOL, &cap,
> +			"attempting to determine the temporary state",
> +			sizeof(cap.stclear_flags)))
> +		goto out_ops;
> +
>   	rc = sprintf(buf, "%d\n", cap.stclear_flags.deactivated);
> +out_ops:
> +	tpm_put_ops(chip);
>   	return rc;
>   }
>   static DEVICE_ATTR_RO(temp_deactivated);
> @@ -195,15 +222,18 @@ static ssize_t caps_show(struct device *dev, struct device_attribute *attr,
>   			 char *buf)
>   {
>   	struct tpm_chip *chip = to_tpm_chip(dev);
> -	cap_t cap;
> -	ssize_t rc;
> +	ssize_t rc = 0;
>   	char *str = buf;
> +	cap_t cap;
>
> -	rc = tpm1_getcap(chip, TPM_CAP_PROP_MANUFACTURER, &cap,
> -			 "attempting to determine the manufacturer",
> -			 sizeof(cap.manufacturer_id));
> -	if (rc)
> +	if (tpm_try_get_ops(chip))
>   		return 0;
> +
> +	if (tpm1_getcap(chip, TPM_CAP_PROP_MANUFACTURER, &cap,
> +			"attempting to determine the manufacturer",
> +			sizeof(cap.manufacturer_id)))
> +		goto out_ops;
> +
>   	str += sprintf(str, "Manufacturer: 0x%x\n",
>   		       be32_to_cpu(cap.manufacturer_id));
>
> @@ -220,11 +250,10 @@ static ssize_t caps_show(struct device *dev, struct device_attribute *attr,
>   			       cap.tpm_version_1_2.revMinor);
>   	} else {
>   		/* Otherwise just use TPM_STRUCT_VER */
> -		rc = tpm1_getcap(chip, TPM_CAP_VERSION_1_1, &cap,
> -				 "attempting to determine the 1.1 version",
> -				 sizeof(cap.tpm_version));
> -		if (rc)
> -			return 0;
> +		if (tpm1_getcap(chip, TPM_CAP_VERSION_1_1, &cap,
> +				"attempting to determine the 1.1 version",
> +				sizeof(cap.tpm_version)))
> +			goto out_ops;
>   		str += sprintf(str,
>   			       "TCG version: %d.%d\nFirmware version: %d.%d\n",
>   			       cap.tpm_version.Major,
> @@ -232,8 +261,10 @@ static ssize_t caps_show(struct device *dev, struct device_attribute *attr,
>   			       cap.tpm_version.revMajor,
>   			       cap.tpm_version.revMinor);
>   	}
> -
> -	return str - buf;
> +	rc = str - buf;
> +out_ops:
> +	tpm_put_ops(chip);
> +	return rc;
>   }
>   static DEVICE_ATTR_RO(caps);
>
> @@ -241,10 +272,12 @@ static ssize_t cancel_store(struct device *dev, struct device_attribute *attr,
>   			    const char *buf, size_t count)
>   {
>   	struct tpm_chip *chip = to_tpm_chip(dev);
> -	if (chip == NULL)
> +
> +	if (tpm_try_get_ops(chip))
>   		return 0;
>
>   	chip->ops->cancel(chip);
> +	tpm_put_ops(chip);
>   	return count;
>   }
>   static DEVICE_ATTR_WO(cancel);