Messages in this thread | | | Date | Fri, 27 Jul 2012 19:45:45 -0500 | Subject | Re: [GIT PULL] New TPM driver, hwrng driver and fixes | From | Kent Yoder <> |
| |
>> + /* err can be positive if it came from the TPM itself, >> + * so return a negative value here instead. */ >> + err = -EFAULT; > > -EFAULT is definitely wrong (that means a bad pointer was passed), you > can use -EIO instead. > > However, I would suggest: > > err = total ? total : -EIO;
This is fine w/ me...
> ... so you report the number of bytes successfully received if we got > any. However, since you *also* do that on the retry line, > >> + 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; > > Should we return something other than 0 if we run out of retries here, too?
Ugh, I was hoping to avoid this kind of complexity.
> Perhaps we should just do the same "err = total ? total : -EIO;" here > and the above statement can just turn into a break;.
Yeah, this seems like the right thing to do.
>> - ret = my_get_random(hash, SHA1_DIGEST_SIZE); >> + ret = tpm_get_random(TPM_ANY_NUM, hash, SHA1_DIGEST_SIZE); >> if (ret < 0) >> return ret; > > You are still not checking the return values correctly!
Dave, can you weigh in on these individual cases? In some cases like capping a pcr I think using uninitialized stack data could be better than failing...
Kent
> This needs to be something like: > > ret = tpm_get_random(TPM_ANY_NUM, hash, SHA1_DIGEST_SIZE); > if (ret != SHA1_DIGEST_SIZE) > return -EIO; /* Or whatever is appropriate here */ > > > -hpa > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
-- IBM LTC Security
| |