| Message ID | 1481077751-106192-1-git-send-email-arei.gonglei@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/06/2016 08:29 PM, Gonglei wrote: > Common practice with sensitive information (key material, passwords, > etc). Prevents sensitive information from being exposed by accident later in > 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. I also think that pointing to earlier commit ids with similar behavior is a good idea; in other words, call out commit 8813800b. So maybe rework this second paragraph to: Sensitive information is sometimes also held in mlocked pages to prevent it being swapped to disk, but qemu in general is not currently taking that level of precaution (see also commit 8813800b). > > Let's zeroize the memory of CryptoDevBackendSymOpInfo structure pointed > for key material security. > > [v2: Stefan perfects the commit message, thanks] The v2 blurb should appear after the --- line, as it is nice for reviewers but a year from now when reading 'git log' we won't care how many versions were on the list, only about the one version in git. > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > hw/virtio/virtio-crypto.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > The commit message may still need improvement, but the maintainer might be willing to do that without needing a v3. At any rate, Reviewed-by: Eric Blake <eblake@redhat.com>
On 07.12.2016 03:29, Gonglei wrote: > Common practice with sensitive information (key material, passwords, > etc). Prevents sensitive information from being exposed by accident later in > 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. > > Let's zeroize the memory of CryptoDevBackendSymOpInfo structure pointed > for key material security. > > [v2: Stefan perfects the commit message, thanks] > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > hw/virtio/virtio-crypto.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) As far as I'm aware, other projects usually have a special memset variation for doing this. That is because compilers may choose to "optimize" memset(p, ...) + free(p) to just the free(p). Having a special zeroizing function that the compiler cannot drop would prevent this. (By the way, C11 provides this functionality with memset_s().) We are not using free() but g_free(), so the danger of a compiler detecting the pattern and "optimizing" it is probably much lower, but still, the possibility exists. Max
Hi Max, > > On 07.12.2016 03:29, Gonglei wrote: > > Common practice with sensitive information (key material, passwords, > > etc). Prevents sensitive information from being exposed by accident later in > > 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. > > > > Let's zeroize the memory of CryptoDevBackendSymOpInfo structure pointed > > for key material security. > > > > [v2: Stefan perfects the commit message, thanks] > > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > > --- > > hw/virtio/virtio-crypto.c | 13 ++++++++++++- > > 1 file changed, 12 insertions(+), 1 deletion(-) > > As far as I'm aware, other projects usually have a special memset > variation for doing this. That is because compilers may choose to > "optimize" memset(p, ...) + free(p) to just the free(p). Having a Actually, I googled this, but I didn't find a definite answer. And The Linux kernel uses kzfree instead of memset + kfree (mm/slab_common.c). void kzfree(const void *p) { size_t ks; void *mem = (void *)p; if (unlikely(ZERO_OR_NULL_PTR(mem))) return; ks = ksize(mem); memset(mem, 0, ks); kfree(mem); } Which is very similar with glib memory management IMO. > special zeroizing function that the compiler cannot drop would prevent > this. (By the way, C11 provides this functionality with memset_s().) > > We are not using free() but g_free(), so the danger of a compiler > detecting the pattern and "optimizing" it is probably much lower, but > still, the possibility exists. > Do we need to think about the compiler optimization here ? Or follow what the linux kernel does. Regards, -Gonglei
> > From: Eric Blake [mailto:eblake@redhat.com] > Sent: Wednesday, December 07, 2016 11:22 PM > To: Gonglei (Arei); qemu-devel@nongnu.org > Cc: mst@redhat.com; stefanha@redhat.com > Subject: Re: [PATCH for-2.9 v2] virtio-crypto: zeroize the key material before > free > > On 12/06/2016 08:29 PM, Gonglei wrote: > > Common practice with sensitive information (key material, passwords, > > etc). Prevents sensitive information from being exposed by accident later in > > 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. > > I also think that pointing to earlier commit ids with similar behavior > is a good idea; in other words, call out commit 8813800b. So maybe > rework this second paragraph to: > > Sensitive information is sometimes also held in mlocked pages to prevent > it being swapped to disk, but qemu in general is not currently taking > that level of precaution (see also commit 8813800b). > > > > > Let's zeroize the memory of CryptoDevBackendSymOpInfo structure pointed > > for key material security. > > > > [v2: Stefan perfects the commit message, thanks] > > The v2 blurb should appear after the --- line, as it is nice for > reviewers but a year from now when reading 'git log' we won't care how > many versions were on the list, only about the one version in git. > Yes, you are right. I just wanted to keep the Stefan's work because the most of commit message comes from him. :) > > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > > --- > > hw/virtio/virtio-crypto.c | 13 ++++++++++++- > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > The commit message may still need improvement, but the maintainer might > be willing to do that without needing a v3. At any rate, > Reviewed-by: Eric Blake <eblake@redhat.com> > Thanks. Regards, -Gonglei
On 12/07/2016 08:33 PM, Gonglei (Arei) wrote: >> Sensitive information is sometimes also held in mlocked pages to prevent >> it being swapped to disk, but qemu in general is not currently taking >> that level of precaution (see also commit 8813800b). >> >>> >>> Let's zeroize the memory of CryptoDevBackendSymOpInfo structure pointed >>> for key material security. >>> >>> [v2: Stefan perfects the commit message, thanks] >> >> The v2 blurb should appear after the --- line, as it is nice for >> reviewers but a year from now when reading 'git log' we won't care how >> many versions were on the list, only about the one version in git. >> > Yes, you are right. I just wanted to keep the Stefan's work because > the most of commit message comes from him. :) Then I might have written: [Thanks to Stefan for help with crafting the commit message] > >>> Signed-off-by: Gonglei <arei.gonglei@huawei.com> >>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> >>> --- >>> hw/virtio/virtio-crypto.c | 13 ++++++++++++- >>> 1 file changed, 12 insertions(+), 1 deletion(-) >>> >> >> The commit message may still need improvement, but the maintainer might >> be willing to do that without needing a v3. At any rate, >> Reviewed-by: Eric Blake <eblake@redhat.com> >> > Thanks. We'll see what the maintainer thinks :)
On 12/07/2016 08:28 PM, Gonglei (Arei) wrote: >> As far as I'm aware, other projects usually have a special memset >> variation for doing this. That is because compilers may choose to >> "optimize" memset(p, ...) + free(p) to just the free(p). Having a > > Actually, I googled this, but I didn't find a definite answer. And > > The Linux kernel uses kzfree instead of memset + kfree (mm/slab_common.c). If we're worried about cleaning things without allowing the compiler a chance to optimize, then writing our own qemu_zfree() wrapper may indeed make sense. But that won't cover the case in Daniel's earlier patch (referenced elsewhere in this thread), as that was zeroizing stack memory (before it went out of scope) rather than heap memory (before free). So you'd still need some sort of 'write this memory no matter what' primitive that would be directly usable on stack memory and indirectly used as part of the qemu_zfree() wrapper. But I wouldn't worry about it for now, unless someone proves we actually have a compiler optimizing away the cleanups.
On 08.12.2016 16:23, Eric Blake wrote: > On 12/07/2016 08:28 PM, Gonglei (Arei) wrote: > >>> As far as I'm aware, other projects usually have a special memset >>> variation for doing this. That is because compilers may choose to >>> "optimize" memset(p, ...) + free(p) to just the free(p). Having a >> >> Actually, I googled this, but I didn't find a definite answer. And >> >> The Linux kernel uses kzfree instead of memset + kfree (mm/slab_common.c). Well, I personally don't mind whether we use a custom zfree() or a custom memset_s() + free(). I only mind that this patch actually always does what it is intended to do. > If we're worried about cleaning things without allowing the compiler a > chance to optimize, then writing our own qemu_zfree() wrapper may indeed > make sense. But that won't cover the case in Daniel's earlier patch > (referenced elsewhere in this thread), as that was zeroizing stack > memory (before it went out of scope) rather than heap memory (before > free). So you'd still need some sort of 'write this memory no matter > what' primitive that would be directly usable on stack memory and > indirectly used as part of the qemu_zfree() wrapper. > > But I wouldn't worry about it for now, unless someone proves we actually > have a compiler optimizing away the cleanups. It's true that we don't have to worry about it *now*, but the approach of waiting until the compiler breaks it does not seem right to me. If at some point some compiler recognizes g_free() as being the same function as free() and thus optimizes memset() + g_free(), we simply won't notice because nobody will keep track of the generated assembly output all the time. There is a reason C11 introduced memset_s(), and it is this. Max
Hi, > > Subject: Re: [Qemu-devel] [PATCH for-2.9 v2] virtio-crypto: zeroize the key > material before free > > On 08.12.2016 16:23, Eric Blake wrote: > > On 12/07/2016 08:28 PM, Gonglei (Arei) wrote: > > > >>> As far as I'm aware, other projects usually have a special memset > >>> variation for doing this. That is because compilers may choose to > >>> "optimize" memset(p, ...) + free(p) to just the free(p). Having a > >> > >> Actually, I googled this, but I didn't find a definite answer. And > >> > >> The Linux kernel uses kzfree instead of memset + kfree > (mm/slab_common.c). > > Well, I personally don't mind whether we use a custom zfree() or a > custom memset_s() + free(). I only mind that this patch actually always > does what it is intended to do. > Yes, but why linux kernel to think about the compiler optimization for memset for sensitive data? > > If we're worried about cleaning things without allowing the compiler a > > chance to optimize, then writing our own qemu_zfree() wrapper may indeed > > make sense. But that won't cover the case in Daniel's earlier patch > > (referenced elsewhere in this thread), as that was zeroizing stack > > memory (before it went out of scope) rather than heap memory (before > > free). So you'd still need some sort of 'write this memory no matter > > what' primitive that would be directly usable on stack memory and > > indirectly used as part of the qemu_zfree() wrapper. > > If we wrap a secure_memset(), then both stack memory and heap memory can use it. Pls see: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1381.pdf But the secure_memset() is not as efficient as possible due to the nature of the volatile type qualifier preventing the compiler from optimizing the code at all. It will cause huge performance regression at hot path of data plane... > > But I wouldn't worry about it for now, unless someone proves we actually > > have a compiler optimizing away the cleanups. > > It's true that we don't have to worry about it *now*, but the approach > of waiting until the compiler breaks it does not seem right to me. > > If at some point some compiler recognizes g_free() as being the same > function as free() and thus optimizes memset() + g_free(), we simply > won't notice because nobody will keep track of the generated assembly > output all the time. > > There is a reason C11 introduced memset_s(), and it is this. > ...does it make sense if we introduce secure_memset() now ? Waiting for your feedback. Thanks! Regards, -Gonglei
On 09.12.2016 02:42, Gonglei (Arei) wrote: > Hi, > >> >> Subject: Re: [Qemu-devel] [PATCH for-2.9 v2] virtio-crypto: zeroize the key >> material before free >> >> On 08.12.2016 16:23, Eric Blake wrote: >>> On 12/07/2016 08:28 PM, Gonglei (Arei) wrote: >>> >>>>> As far as I'm aware, other projects usually have a special memset >>>>> variation for doing this. That is because compilers may choose to >>>>> "optimize" memset(p, ...) + free(p) to just the free(p). Having a >>>> >>>> Actually, I googled this, but I didn't find a definite answer. And >>>> >>>> The Linux kernel uses kzfree instead of memset + kfree >> (mm/slab_common.c). >> >> Well, I personally don't mind whether we use a custom zfree() or a >> custom memset_s() + free(). I only mind that this patch actually always >> does what it is intended to do. >> > Yes, but why linux kernel to think about the compiler optimization for > memset for sensitive data? I'm afraid I don't quite (syntactically) understand this question. Do you mean to ask why the Linux kernel would have to think about this optimization? My answer to that would be because the optimization of memset() + free() is known, and they probably want to protect against the compiler optimizing it even with -ffreestanding and differently called functions -- you never know. Related example: gcc detects code that basically does a memset() and replaces it with a call to memset(). There were a couple of versions where it did that even with -ffreestanding or -fno-builtin, which is bad if you want to actually write a memset(). They fixed it by now (so that -ffreestanding and -fno-builtin will imply -fno-tree-loop-distribute-patterns, which will disable that optimization. Now imagine the same case here: Maybe at some point gcc is able to detect that kfree() is basically free() and will then optimize the memset() before kfree() away. A couple of versions later, somebody will notice that -- but at that point, the damage has been done already and there are compiled versions out which leak sensitive data somewhere. I think this is why the Linux kernel decides to be proactive and use kzfree() from the start, and this is also why I'm proposing to not delay this issue in qemu until some compiler actually makes it a real issue. >>> If we're worried about cleaning things without allowing the compiler a >>> chance to optimize, then writing our own qemu_zfree() wrapper may indeed >>> make sense. But that won't cover the case in Daniel's earlier patch >>> (referenced elsewhere in this thread), as that was zeroizing stack >>> memory (before it went out of scope) rather than heap memory (before >>> free). So you'd still need some sort of 'write this memory no matter >>> what' primitive that would be directly usable on stack memory and >>> indirectly used as part of the qemu_zfree() wrapper. >>> > If we wrap a secure_memset(), then both stack memory and heap > memory can use it. > > Pls see: > > http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1381.pdf > > But the secure_memset() is not as efficient as possible due to the nature of the volatile > type qualifier preventing the compiler from optimizing the code at all. > > It will cause huge performance regression at hot path of data plane... I would suggest we implement an own secure_memset() or secure_bzero() which falls back on what we have: - memset_s(), if that is available; - explizit_bzero() if we have libbsd; - SecureZeroMemory() on Windows Or we have to write it ourselves: https://sourceware.org/ml/libc-alpha/2014-12/msg00506.html suggests that putting a full memory barrier after the memset() should be enough. >>> But I wouldn't worry about it for now, unless someone proves we actually >>> have a compiler optimizing away the cleanups. >> >> It's true that we don't have to worry about it *now*, but the approach >> of waiting until the compiler breaks it does not seem right to me. >> >> If at some point some compiler recognizes g_free() as being the same >> function as free() and thus optimizes memset() + g_free(), we simply >> won't notice because nobody will keep track of the generated assembly >> output all the time. >> >> There is a reason C11 introduced memset_s(), and it is this. >> > ...does it make sense if we introduce secure_memset() now ? > > Waiting for your feedback. Thanks! I would propose so; or better something like secure_bzero() or secure_memzero(), because we can then fall back on existing implementations like explizit_bzero() or SecureZeroMemory(). We don't have to implement this immediately, but we should do it before the 2.9 release. Max
> > > On 09.12.2016 02:42, Gonglei (Arei) wrote: > > Hi, > > > >> > >> Subject: Re: [Qemu-devel] [PATCH for-2.9 v2] virtio-crypto: zeroize the key > >> material before free > >> > >> On 08.12.2016 16:23, Eric Blake wrote: > >>> On 12/07/2016 08:28 PM, Gonglei (Arei) wrote: > >>> > >>>>> As far as I'm aware, other projects usually have a special memset > >>>>> variation for doing this. That is because compilers may choose to > >>>>> "optimize" memset(p, ...) + free(p) to just the free(p). Having a > >>>> > >>>> Actually, I googled this, but I didn't find a definite answer. And > >>>> > >>>> The Linux kernel uses kzfree instead of memset + kfree > >> (mm/slab_common.c). > >> > >> Well, I personally don't mind whether we use a custom zfree() or a > >> custom memset_s() + free(). I only mind that this patch actually always > >> does what it is intended to do. > >> > > Yes, but why linux kernel to think about the compiler optimization for > > memset for sensitive data? > > I'm afraid I don't quite (syntactically) understand this question. Do > you mean to ask why the Linux kernel would have to think about this > optimization? My answer to that would be because the optimization of > memset() + free() is known, and they probably want to protect against > the compiler optimizing it even with -ffreestanding and differently > called functions -- you never know. > Sorry, that's my typo. ;) My question is why not linux kernel to think about the compiler optimization on the code layer. Because the realization of kzfree is just memset + kfree, it didn't add any memory barrier to prevent memset is optimized, or use secure_memset, or whatever. So you answer means they maybe use some compiler options to avoid the compiler optimizing? > Related example: gcc detects code that basically does a memset() and > replaces it with a call to memset(). There were a couple of versions > where it did that even with -ffreestanding or -fno-builtin, which is bad > if you want to actually write a memset(). They fixed it by now (so that > -ffreestanding and -fno-builtin will imply > -fno-tree-loop-distribute-patterns, which will disable that optimization. > > Now imagine the same case here: Maybe at some point gcc is able to > detect that kfree() is basically free() and will then optimize the > memset() before kfree() away. A couple of versions later, somebody will > notice that -- but at that point, the damage has been done already and > there are compiled versions out which leak sensitive data somewhere. > > I think this is why the Linux kernel decides to be proactive and use > kzfree() from the start, and this is also why I'm proposing to not delay > this issue in qemu until some compiler actually makes it a real issue. > > >>> If we're worried about cleaning things without allowing the compiler a > >>> chance to optimize, then writing our own qemu_zfree() wrapper may > indeed > >>> make sense. But that won't cover the case in Daniel's earlier patch > >>> (referenced elsewhere in this thread), as that was zeroizing stack > >>> memory (before it went out of scope) rather than heap memory (before > >>> free). So you'd still need some sort of 'write this memory no matter > >>> what' primitive that would be directly usable on stack memory and > >>> indirectly used as part of the qemu_zfree() wrapper. > >>> > > If we wrap a secure_memset(), then both stack memory and heap > > memory can use it. > > > > Pls see: > > > > http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1381.pdf > > > > But the secure_memset() is not as efficient as possible due to the nature of > the volatile > > type qualifier preventing the compiler from optimizing the code at all. > > > > It will cause huge performance regression at hot path of data plane... > > I would suggest we implement an own secure_memset() or secure_bzero() > which falls back on what we have: > - memset_s(), if that is available; > - explizit_bzero() if we have libbsd; > - SecureZeroMemory() on Windows > > Or we have to write it ourselves: > https://sourceware.org/ml/libc-alpha/2014-12/msg00506.html suggests that > putting a full memory barrier after the memset() should be enough. > I like this approach. > >>> But I wouldn't worry about it for now, unless someone proves we actually > >>> have a compiler optimizing away the cleanups. > >> > >> It's true that we don't have to worry about it *now*, but the approach > >> of waiting until the compiler breaks it does not seem right to me. > >> > >> If at some point some compiler recognizes g_free() as being the same > >> function as free() and thus optimizes memset() + g_free(), we simply > >> won't notice because nobody will keep track of the generated assembly > >> output all the time. > >> > >> There is a reason C11 introduced memset_s(), and it is this. > >> > > ...does it make sense if we introduce secure_memset() now ? > > > > Waiting for your feedback. Thanks! > > I would propose so; or better something like secure_bzero() or > secure_memzero(), because we can then fall back on existing > implementations like explizit_bzero() or SecureZeroMemory(). > Both OK. > We don't have to implement this immediately, but we should do it before > the 2.9 release. > Agree. Regards, -Gonglei
On 10.12.2016 03:58, Gonglei (Arei) wrote: >> >> >> On 09.12.2016 02:42, Gonglei (Arei) wrote: >>> Hi, >>> >>>> >>>> Subject: Re: [Qemu-devel] [PATCH for-2.9 v2] virtio-crypto: zeroize the key >>>> material before free >>>> >>>> On 08.12.2016 16:23, Eric Blake wrote: >>>>> On 12/07/2016 08:28 PM, Gonglei (Arei) wrote: >>>>> >>>>>>> As far as I'm aware, other projects usually have a special memset >>>>>>> variation for doing this. That is because compilers may choose to >>>>>>> "optimize" memset(p, ...) + free(p) to just the free(p). Having a >>>>>> >>>>>> Actually, I googled this, but I didn't find a definite answer. And >>>>>> >>>>>> The Linux kernel uses kzfree instead of memset + kfree >>>> (mm/slab_common.c). >>>> >>>> Well, I personally don't mind whether we use a custom zfree() or a >>>> custom memset_s() + free(). I only mind that this patch actually always >>>> does what it is intended to do. >>>> >>> Yes, but why linux kernel to think about the compiler optimization for >>> memset for sensitive data? >> >> I'm afraid I don't quite (syntactically) understand this question. Do >> you mean to ask why the Linux kernel would have to think about this >> optimization? My answer to that would be because the optimization of >> memset() + free() is known, and they probably want to protect against >> the compiler optimizing it even with -ffreestanding and differently >> called functions -- you never know. >> > Sorry, that's my typo. ;) > > My question is why not linux kernel to think about the compiler optimization > on the code layer. Because the realization of kzfree is just memset + kfree, > it didn't add any memory barrier to prevent memset is optimized, or use > secure_memset, or whatever. > > So you answer means they maybe use some compiler options to avoid > the compiler optimizing? Yes, maybe they are trusting that using -ffreestanding will keep the compiler from optimizing the combination. I personally wouldn't trust that, because I have seen gcc ignore that convention and just optimize anyway, but I'm not Linus and I guess if Linus complains about gcc breaking something, it will be fixed much more quickly than when I do. Max
On Thu, Dec 08, 2016 at 09:20:07AM -0600, Eric Blake wrote: > On 12/07/2016 08:33 PM, Gonglei (Arei) wrote: > > >> Sensitive information is sometimes also held in mlocked pages to prevent > >> it being swapped to disk, but qemu in general is not currently taking > >> that level of precaution (see also commit 8813800b). > >> > >>> > >>> Let's zeroize the memory of CryptoDevBackendSymOpInfo structure pointed > >>> for key material security. > >>> > >>> [v2: Stefan perfects the commit message, thanks] > >> > >> The v2 blurb should appear after the --- line, as it is nice for > >> reviewers but a year from now when reading 'git log' we won't care how > >> many versions were on the list, only about the one version in git. > >> > > Yes, you are right. I just wanted to keep the Stefan's work because > > the most of commit message comes from him. :) > > Then I might have written: > > [Thanks to Stefan for help with crafting the commit message] > > > > >>> Signed-off-by: Gonglei <arei.gonglei@huawei.com> > >>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > >>> --- > >>> hw/virtio/virtio-crypto.c | 13 ++++++++++++- > >>> 1 file changed, 12 insertions(+), 1 deletion(-) > >>> > >> > >> The commit message may still need improvement, but the maintainer might > >> be willing to do that without needing a v3. At any rate, > >> Reviewed-by: Eric Blake <eblake@redhat.com> > >> > > Thanks. > > We'll see what the maintainer thinks :) > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org > I'd suggest post v3 after 2.8 is out.
> -----Original Message----- > From: Michael S. Tsirkin [mailto:mst@redhat.com] > Sent: Sunday, December 11, 2016 10:51 AM > To: Eric Blake > Cc: Gonglei (Arei); qemu-devel@nongnu.org; stefanha@redhat.com > Subject: Re: [PATCH for-2.9 v2] virtio-crypto: zeroize the key material before > free > > On Thu, Dec 08, 2016 at 09:20:07AM -0600, Eric Blake wrote: > > On 12/07/2016 08:33 PM, Gonglei (Arei) wrote: > > > > >> Sensitive information is sometimes also held in mlocked pages to prevent > > >> it being swapped to disk, but qemu in general is not currently taking > > >> that level of precaution (see also commit 8813800b). > > >> > > >>> > > >>> Let's zeroize the memory of CryptoDevBackendSymOpInfo structure > pointed > > >>> for key material security. > > >>> > > >>> [v2: Stefan perfects the commit message, thanks] > > >> > > >> The v2 blurb should appear after the --- line, as it is nice for > > >> reviewers but a year from now when reading 'git log' we won't care how > > >> many versions were on the list, only about the one version in git. > > >> > > > Yes, you are right. I just wanted to keep the Stefan's work because > > > the most of commit message comes from him. :) > > > > Then I might have written: > > > > [Thanks to Stefan for help with crafting the commit message] > > > > > > > >>> Signed-off-by: Gonglei <arei.gonglei@huawei.com> > > >>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > > >>> --- > > >>> hw/virtio/virtio-crypto.c | 13 ++++++++++++- > > >>> 1 file changed, 12 insertions(+), 1 deletion(-) > > >>> > > >> > > >> The commit message may still need improvement, but the maintainer > might > > >> be willing to do that without needing a v3. At any rate, > > >> Reviewed-by: Eric Blake <eblake@redhat.com> > > >> > > > Thanks. > > > > We'll see what the maintainer thinks :) > > > > -- > > Eric Blake eblake redhat com +1-919-301-3266 > > Libvirt virtualization library http://libvirt.org > > > > I'd suggest post v3 after 2.8 is out. > OK, will do. Regards, -Gonglei
diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c index 2f2467e..ecb19b6 100644 --- a/hw/virtio/virtio-crypto.c +++ b/hw/virtio/virtio-crypto.c @@ -337,7 +337,18 @@ static void virtio_crypto_free_request(VirtIOCryptoReq *req) { if (req) { if (req->flags == CRYPTODEV_BACKEND_ALG_SYM) { - g_free(req->u.sym_op_info); + size_t max_len; + CryptoDevBackendSymOpInfo *op_info = req->u.sym_op_info; + + max_len = op_info->iv_len + + op_info->aad_len + + op_info->src_len + + op_info->dst_len + + op_info->digest_result_len; + + /* Zeroize and free request data structure */ + memset(op_info, 0, sizeof(*op_info) + max_len); + g_free(op_info); } g_free(req); }