Re: [PATCH v4 2/7] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling

From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: James Bottomley <James.Bottomley@hansenpartnership.com>,
	linux-integrity <linux-integrity@vger.kernel.org>,
	"open list:HARDWARE RANDOM NUMBER GENERATOR CORE"
	<linux-crypto@vger.kernel.org>,
	linux-security-module <linux-security-module@vger.kernel.org>,
	Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Subject: Re: [PATCH v4 2/7] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling
Date: Wed, 24 Oct 2018 11:40:49 +0300 (EEST)	[thread overview]
Message-ID: <alpine.DEB.2.21.1810241137400.3525@jsakkine-mobl1> (raw)
In-Reply-To: <CAKv+Gu8T8yGEu2GWWKzLXR=wgd=mYHcTYLrD69_zpaddQK3Zxw@mail.gmail.com>

On Tue, 23 Oct 2018, Ard Biesheuvel wrote:
> On 23 October 2018 at 04:01, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
>> On Mon, 2018-10-22 at 19:19 -0300, Ard Biesheuvel wrote:
>> [...]
>>>> +static void hmac_init(struct shash_desc *desc, u8 *key, int
>>>> keylen)
>>>> +{
>>>> +       u8 pad[SHA256_BLOCK_SIZE];
>>>> +       int i;
>>>> +
>>>> +       desc->tfm = sha256_hash;
>>>> +       desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
>>>
>>> I don't think this actually does anything in the shash API
>>> implementation, so you can drop this.
>>
>> OK, I find crypto somewhat hard to follow.  There were bits I had to
>> understand, like when I wrote the CFB implementation or when I fixed
>> the ECDH scatterlist handling, but I've got to confess, in time
>> honoured tradition I simply copied this from EVM crypto without
>> actually digging into the code to understand why.
>>
>
> Yeah, it is notoriously hard to use, and we should try to improve that.

James,

I would hope (already said in my review) to use longer than one
character variable names for most of the stuff. I did not quite
understand why you decided to use 'counter' for obvious counter
variable and one character names for non-obvious stuff :-)

I'm not sure where the 'encoded' exactly comes in the variable
name 'encoded_key' especially in the context of these cryptic
names.

/Jarkko