Re: [GIT PULL] New TPM driver, hwrng driver and fixes

["H. Peter Anvin" <hpa@zytor.com>]

On 07/27/2012 11:14 AM, Kent Yoder wrote:
> Hi James,
> 
> Please pull from this new branch and ignore the 7-25-12 branch. This
> new branch includes fixes for comments by hpa. I've also included one
> additional patch from [1] to close a race and prevent possibly sensitive
> data from being free'd before being zeroed. I'm attaching this entire
> diff here since my fixes for hpa's comments aren't public yet.
> 
> Thanks,
> Kent

> +
> +/**
> + * tpm_get_random() - Get random bytes from the tpm's RNG
> + * @chip_num: A specific chip number for the request or TPM_ANY_NUM
> + * @out: destination buffer for the random bytes
> + * @max: on input, the max number of bytes to write to @out, on output
> + *       this is set to the actual number of bytes written to @out
> + *
> + * Note that @max will be capped at TPM_MAX_RNG_DATA bytes.
> + */
> +int tpm_get_random(u32 chip_num, u8 *out, size_t *max)
> +{
> +	struct tpm_chip *chip;
> +	struct tpm_cmd_t tpm_cmd;
> +	u32 recd, total = 0, num_bytes = min_t(u32, *max, TPM_MAX_RNG_DATA);
> +	int err, retries = 5;
> +	u8 *dest = out;
> +
> +	chip = tpm_chip_find_get(chip_num);
> +	if (chip == NULL)
> +		return -ENODEV;
> +
> +	if (!out || !num_bytes || *max > TPM_MAX_RN
> +		return -EINVAL;
> +
> +	do {
> +		tpm_cmd.header.in = tpm_getrandom_header;
> +		tpm_cmd.params.getrandom_in.num_bytes = cpu_to_be32(num_bytes);
> +
> +		err = transmit_cmd(chip, &tpm_cmd,
> +				TPM_GETRANDOM_RESULT_SIZE + num_bytes,
> +				"attempting get random");
> +		if (err)
> +			goto out_err;
> +
> +		recd = be32_to_cpu(tpm_cmd.params.getrandom_out.rng_data_len);
> +		memcpy(dest, tpm_cmd.params.getrandom_out.rng_data, recd);
> +
> +		dest += recd;
> +		total += recd;
> +		num_bytes -= recd;
> +	} while (retries-- && total < *max);
> +
> +	err = ((total < *max) ? -EAGAIN : 0);
> +out_err:
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(tpm_get_random);
> +

Since you no longer modify *max anywhere in this function, why leave it
a pointer?  Making it pass by value seems more logical at that point
(and cleaner).

The only consumer which can make use of partial result is
tpm_rng_read(), but that will now return zero unless the buffer is filled.

My suggestion would be to drop the pointer and instead return a positive
result (number of bytes) if you read anything and a negative result
(-errno) on error.

That way, a caller which can only use an exact number of bytes should do:

	rv = tpm_get_random(chip, buf, size);
	if (rv != size)
		/* error! */

... and tpm_rng_read() can do something smarter.

	-hpa