Re: [PATCH] mac80211: aes_ccm: move struct aead_req off the stack

[Ard Biesheuvel]

> On 14 Oct 2016, at 14:46, Johannes Berg  wrote:
> 
> 
>> 
>> Is the aad[] actually reused? I would assume it only affects the mac
>> on encryption, and the verification on decryption but I don't think
>> we actually need it back from the crypto routines.
> 
> I don't think it's reused.
> 
>> Exactly what you said above :-) My patch only touches CCM but as you
>> said,
>> 
>> """
>> 'Also there's B_0/J_0 for CCM/GCM, and the 'zero' thing that GMAC
>> has.
>> """
> 
> Ah, but we can/should do the same for the others, no?
> 

Yes, but then we end up kmalloc/kfreeing chunks of 16 bytes, which is actually 
another problem.

I still think we are not violating the api by putting aead_req on the stack 
(but herbert should confirm). The aad[] issue does violate the api, so it 
deserves a separate fix imo

Re: [PATCH] mac80211: aes_ccm: move struct aead_req off the stack

[Johannes Berg]

> 
> Is the aad[] actually reused? I would assume it only affects the mac
> on encryption, and the verification on decryption but I don't think
> we actually need it back from the crypto routines.

I don't think it's reused.

> Exactly what you said above :-) My patch only touches CCM but as you
> said,
> 
> """
> 'Also there's B_0/J_0 for CCM/GCM, and the 'zero' thing that GMAC
> has.
> """

Ah, but we can/should do the same for the others, no?

johannes

Re: [PATCH] mac80211: aes_ccm: move struct aead_req off the stack

[Johannes Berg]

On Fri, 2016-10-14 at 14:13 +0100, Ard Biesheuvel wrote:
> 
> > But if we allocate things anyway, is it worth expending per-CPU
> > buffers on these?
> 
> Ehmm, maybe not. I could spin a v2 that allocates a bigger buffer,
> and copies aad[] into it as well

Copies in/out, I guess. Also there's B_0/J_0 for CCM/GCM, and the
'zero' thing that GMAC has.

> That does not help the other algos though

What do you mean?

johannes

Re: [PATCH] mac80211: aes_ccm: move struct aead_req off the stack

[Ard Biesheuvel]

On 14 October 2016 at 14:15, Johannes Berg  wrote:
> On Fri, 2016-10-14 at 14:13 +0100, Ard Biesheuvel wrote:
>>
>> > But if we allocate things anyway, is it worth expending per-CPU
>> > buffers on these?
>>
>> Ehmm, maybe not. I could spin a v2 that allocates a bigger buffer,
>> and copies aad[] into it as well
>
> Copies in/out, I guess. Also there's B_0/J_0 for CCM/GCM, and the
> 'zero' thing that GMAC has.
>

Is the aad[] actually reused? I would assume it only affects the mac
on encryption, and the verification on decryption but I don't think we
actually need it back from the crypto routines.

>> That does not help the other algos though
>
> What do you mean?
>

Exactly what you said above :-) My patch only touches CCM but as you said,

"""
'Also there's B_0/J_0 for CCM/GCM, and the 'zero' thing that GMAC has.
"""

Re: [PATCH] mac80211: aes_ccm: move struct aead_req off the stack

[Johannes Berg]

On Fri, 2016-10-14 at 15:10 +0200, Johannes Berg wrote:
> > 
> > So use kzalloc
> 
> Do we really need kzalloc()? We have things on the stack right now,
> and don't initialize, so surely we don't really need to zero things? 

Err, never mind, I'm an idiot - we *do* initialize to 0, of course.

johannes

Re: [PATCH] mac80211: aes_ccm: move struct aead_req off the stack

[Ard Biesheuvel]

On 14 October 2016 at 14:10, Johannes Berg  wrote:
>
>> So use kzalloc
>
> Do we really need kzalloc()? We have things on the stack right now, and
> don't initialize, so surely we don't really need to zero things?
>
>> This only addresses one half of the problem. The other problem, i.e.,
>> the fact that the aad[] array lives on the stack of the caller, is
>> handled adequately imo by the change proposed by Johannes.
>
> But if we allocate things anyway, is it worth expending per-CPU buffers
> on these?
>

Ehmm, maybe not. I could spin a v2 that allocates a bigger buffer, and
copies aad[] into it as well
That does not help the other algos though