| Message ID | 1481016553-69252-1-git-send-email-arei.gonglei@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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>
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>
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.
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
> > 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
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); }
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(-)