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

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

[Gonglei]

Zeroize the memory of CryptoDevBackendSymOpInfo structure pointed
for key material security.

Signed-off-by: Gonglei <arei.gonglei@huawei.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.8] virtio-crypto: zeroize the key material before free

[Michael S. Tsirkin]

On Tue, Dec 06, 2016 at 05:29:13PM +0800, Gonglei wrote:
> Zeroize the memory of CryptoDevBackendSymOpInfo structure pointed
> for key material security.
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.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);

Write into memory, then free it?  This looks rather strange. Why are we
doing this?

>          }
>          g_free(req);
>      }
> -- 
> 1.8.3.1
> 

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

[Stefan Hajnoczi]

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

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.
> > 
> > Signed-off-by: Gonglei <arei.gonglei@huawei.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);
> 
> Write into memory, then free it?  This looks rather strange. Why are we
> doing this?

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.

Perhaps the comment should be more explicit but this patch seems
reasonable.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

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

[Michael S. Tsirkin]

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.
> > > 
> > > Signed-off-by: Gonglei <arei.gonglei@huawei.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);
> > 
> > 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.

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

> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

[Eric Blake]

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

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


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

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

[Stefan Hajnoczi]

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

On Tue, Dec 06, 2016 at 01:30:12PM -0600, Eric Blake wrote:
> 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).

Okay.  I am not merging this for QEMU 2.8.0-rc3, it should go through
Michael Tsirkin's tree.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

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

[Gonglei (Arei)]

>
> From: Stefan Hajnoczi [mailto:stefanha@gmail.com]
> Sent: Wednesday, December 07, 2016 4:18 AM
> To: Eric Blake
> Cc: Michael S. Tsirkin; Gonglei (Arei); qemu-devel@nongnu.org; Daniel P.
> Berrange
> Subject: Re: [Qemu-devel] [PATCH for-2.8] virtio-crypto: zeroize the key
> material before free
> 
> On Tue, Dec 06, 2016 at 01:30:12PM -0600, Eric Blake wrote:
> > 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).
> 
> Okay.  I am not merging this for QEMU 2.8.0-rc3, it should go through
> Michael Tsirkin's tree.
> 
It's fair, let me send V2 for 2.9 with clearer commit message.

Thank you, guys~

Regards,
-Gonglei