[Qemu-devel] [PATCH for-2.9 v2] virtio-crypto: zeroize the key material before free

[Qemu-devel] [PATCH for-2.9 v2] virtio-crypto: zeroize the key material before free

[Gonglei]

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(-)

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);
     }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH for-2.9 v2] virtio-crypto: zeroize the key material before free

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1665 bytes --]

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>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for-2.9 v2] virtio-crypto: zeroize the key material before free

[Gonglei (Arei)]

>
> 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

Re: [Qemu-devel] [PATCH for-2.9 v2] virtio-crypto: zeroize the key material before free

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1465 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for-2.9 v2] virtio-crypto: zeroize the key material before free

[Michael S. Tsirkin]

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.

-- 
MST

Re: [Qemu-devel] [PATCH for-2.9 v2] virtio-crypto: zeroize the key material before free

[Gonglei (Arei)]

> -----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

Re: [Qemu-devel] [PATCH for-2.9 v2] virtio-crypto: zeroize the key material before free

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1304 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH for-2.9 v2] virtio-crypto: zeroize the key material before free

[Gonglei (Arei)]

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

Re: [Qemu-devel] [PATCH for-2.9 v2] virtio-crypto: zeroize the key material before free

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1197 bytes --]

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for-2.9 v2] virtio-crypto: zeroize the key material before free

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1783 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH for-2.9 v2] virtio-crypto: zeroize the key material before free

[Gonglei (Arei)]

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

Re: [Qemu-devel] [PATCH for-2.9 v2] virtio-crypto: zeroize the key material before free

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 4718 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH for-2.9 v2] virtio-crypto: zeroize the key material before free

[Gonglei (Arei)]

>
> 
> 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

Re: [Qemu-devel] [PATCH for-2.9 v2] virtio-crypto: zeroize the key material before free

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2232 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]