On 12/06/2016 01:22 PM, Michael S. Tsirkin wrote: > On Tue, Dec 06, 2016 at 05:33:37PM +0000, Stefan Hajnoczi wrote: >> On Tue, Dec 06, 2016 at 03:40:49PM +0200, Michael S. Tsirkin wrote: >>> On Tue, Dec 06, 2016 at 05:29:13PM +0800, Gonglei wrote: >>>> Zeroize the memory of CryptoDevBackendSymOpInfo structure pointed >>>> for key material security. >>>>
>>>> + /* Zeroize and free request data structure */ >>>> + memset(op_info, 0, sizeof(*op_info) + max_len); >>>> + g_free(op_info); >>> >>> Write into memory, then free it? This looks rather strange. Why are we >>> doing this? >> >> Common practice with sensitive information (key material, passwords, >> etc). > > Some kind of explanation about what makes this one > more sensitive than others would be nice. Even mentioning existing practice would go a long way; see commit 8813800b. > > Also, what makes it 2.8 material? Considering the pointer math > involved, it's not risk-free. > >> coredumps, memory disclosure bugs when heap memory is reused, etc. >> >> Sensitive information is sometimes also held in mlocked pages to prevent >> it being swapped to disk but that's not being done here. And existing practice is that we aren't going to be that paranoid at this time (and yes, I asked Dan that same question on his commit mentioned above). >> >> Perhaps the comment should be more explicit but this patch seems >> reasonable. > > Right. One can see memset and free at a glance. > The comment and the commit log should explain the why, > not the what. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature