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
