On 08-11-2016 16:45, Stephan Mueller wrote:
> Am Donnerstag, 27. Oktober 2016, 15:36:08 CET schrieb Harsh Jain:
>
> Hi Harsh,
>
>>>> +static void chcr_verify_tag(struct aead_request *req, u8 *input, int
>>>> *err)
>>>> +{
>>>> + u8 temp[SHA512_DIGEST_SIZE];
>>>> + struct crypto_aead *tfm = crypto_aead_reqtfm(req);
>>>> + int authsize = crypto_aead_authsize(tfm);
>>>> + struct cpl_fw6_pld *fw6_pld;
>>>> + int cmp = 0;
>>>> +
>>>> + fw6_pld = (struct cpl_fw6_pld *)input;
>>>> + if ((get_aead_subtype(tfm) == CRYPTO_ALG_SUB_TYPE_AEAD_RFC4106) ||
>>>> + (get_aead_subtype(tfm) == CRYPTO_ALG_SUB_TYPE_AEAD_GCM)) {
>>>> + cmp = memcmp(&fw6_pld->data[2], (fw6_pld + 1), authsize);
>>>> + } else {
>>>> +
>>>> + sg_pcopy_to_buffer(req->src, sg_nents(req->src), temp,
>>>> + authsize, req->assoclen +
>>>> + req->cryptlen - authsize);
>>> I am wondering whether the math is correct here in any case. It is
>>> permissible that we have an AAD size of 0 and even a zero-sized
>>> ciphertext. How is such scenario covered here?
>> Here we are trying to copy user supplied tag to local buffer(temp) for
>> decrypt operation only. relative index of tag in src sg list will not
>> change when AAD is zero and in decrypt operation cryptlen > authsize.
> I am just wondering where this is checked. Since all of these implementations
> are directly accessible from unprivileged user space, we should be careful.
chcr_verify_tag() will be called when req->verify is set to "VERIFY_SW", same will set in decrypt callback function of Algo(like chcr_aead_decrypt) only. It will ensure calling of chcr_verify_tag() in de-crypt operation only.
>
>>>> + cmp = memcmp(temp, (fw6_pld + 1), authsize);
>>> I would guess in both cases memcmp should be replaced with crypto_memneq
>> Yes can be done
>>
>>>> + }
>>>> + if (cmp)
>>>> + *err = -EBADMSG;
>>>> + else
>>>> + *err = 0;
>>> What do you think about memzero_explicit(tmp)?
>> No Idea why we needs explicitly setting of zero for local variable. Please
>> share some online resources to understand this.
> In dumps, the stack is also produced. Yet I see that stack memory is very
> volatile and thus will be overwritten soon. Thus my common approach for
> sensitive data is that heap variables must be zeroized. Stack variables are
> suggested to be zeroized. As far as I understand the code, temp will hold a
> copy of the tag value, i.e. a public piece of information. If this is correct,
> that I concur that a memset may not be needed after all.
Yes, temp contains user supplied tag. We can ignore memset here. I will review the other function weather they need similar memset or not.
>
> Ciao
> Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
