Re: [mac80211] BUG_ON with current -git (4.8.0-11417-g24532f7)

[Ard Biesheuvel]

On 14 October 2016 at 09:39, Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> wrote:
> On 14 October 2016 at 09:28, Johannes Berg <johannes@xxxxxxxxxxxxxxxx> wrote:
>>
>>>    1. revert that patch (doing so would need some major adjustments now,
>>>       since it's pretty old and a number of new things were added in the
>>>       meantime)
>>
>> This it will have to be, I guess.
>>
>>>    2. allocate a per-CPU buffer for all the things that we put on the
>>>       stack and use in SG lists, those are:
>>>        * CCM/GCM: AAD (32B), B_0/J_0 (16B)
>>>        * GMAC: AAD (20B), zero (16B)
>>>        * (not sure why CMAC isn't using this API, but it would be like GMAC)
>>
>> This doesn't work - I tried to move the mac80211 buffers, but because
>> we also put the struct aead_request on the stack, and crypto_ccm has
>> the "odata" in there, and we can't separate the odata from that struct,
>> we'd have to also put that into a per-CPU buffer, but it's very big -
>> 456 bytes for CCM, didn't measure the others but I'd expect them to be
>> larger, if different.
>>
>> I don't think we can allocate half a kb for each CPU just to be able to
>> possibly use the acceleration here. We can't even make that conditional
>> on not having hardware crypto in the wifi NIC because drivers are
>> always allowed to pass undecrypted frames, regardless of whether or not
>> HW crypto was attempted, so we don't know upfront if we'll have to
>> decrypt anything in software...
>>
>> Given that, I think we have had a bug in here basically since Ard's
>> patch, we never should've put these structs on the stack. Herbert, you
>> also touched this later and converted the API usage, did you see the
>> way the stack is used here and think it should be OK, or did you simply
>> not realize that?
>>
>> Ard, are you able to help out working on a revert of your patch? That
>> would require also reverting a number of other patches (various fixes,
>> API adjustments, etc. to the AEAD usage), but the more complicated part
>> is that in the meantime Jouni introduced GCMP and CCMP-256, both of
>> which we of course need to retain.
>>
>
> I am missing some context here, but could you explain what exactly is
> the problem here?
>
> Look at this code
>
> """
> struct scatterlist sg[3];
>
> char aead_req_data[sizeof(struct aead_request) +
>     crypto_aead_reqsize(tfm)]
>     __aligned(__alignof__(struct aead_request));
> struct aead_request *aead_req = (void *) aead_req_data;
>
> memset(aead_req, 0, sizeof(aead_req_data));
>
> sg_init_table(sg, 3);
> sg_set_buf(&sg[0], &aad[2], be16_to_cpup((__be16 *)aad));
> sg_set_buf(&sg[1], data, data_len);
> sg_set_buf(&sg[2], mic, mic_len);
>
> aead_request_set_tfm(aead_req, tfm);
> aead_request_set_crypt(aead_req, sg, sg, data_len, b_0);
> aead_request_set_ad(aead_req, sg[0].length);
> """
>
> I assume the stack buffer itself is not the problem here, but aad,
> which is allocated on the stack one frame up.
> Do we really need to revert the whole patch to fix that?

Ah never mind, this is about 'odata'. Apologies, should have read first