Set to zero the DH context buffers containing the DH key before they are
freed.
Cc: [email protected]
Fixes: c9839143ebbf ("crypto: qat - Add DH support")
Signed-off-by: Giovanni Cabiddu <[email protected]>
Reviewed-by: Adam Guerin <[email protected]>
Reviewed-by: Wojciech Ziemba <[email protected]>
---
drivers/crypto/qat/qat_common/qat_asym_algs.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/crypto/qat/qat_common/qat_asym_algs.c b/drivers/crypto/qat/qat_common/qat_asym_algs.c
index d75eb77c9fb9..2fec89b8a188 100644
--- a/drivers/crypto/qat/qat_common/qat_asym_algs.c
+++ b/drivers/crypto/qat/qat_common/qat_asym_algs.c
@@ -421,14 +421,17 @@ static int qat_dh_set_params(struct qat_dh_ctx *ctx, struct dh *params)
static void qat_dh_clear_ctx(struct device *dev, struct qat_dh_ctx *ctx)
{
if (ctx->g) {
+ memzero_explicit(ctx->g, ctx->p_size);
dma_free_coherent(dev, ctx->p_size, ctx->g, ctx->dma_g);
ctx->g = NULL;
}
if (ctx->xa) {
+ memzero_explicit(ctx->xa, ctx->p_size);
dma_free_coherent(dev, ctx->p_size, ctx->xa, ctx->dma_xa);
ctx->xa = NULL;
}
if (ctx->p) {
+ memzero_explicit(ctx->p, ctx->p_size);
dma_free_coherent(dev, ctx->p_size, ctx->p, ctx->dma_p);
ctx->p = NULL;
}
--
2.35.1
On Fri, May 06, 2022 at 11:01:17AM +0100, Giovanni Cabiddu wrote:
> On Fri, May 06, 2022 at 11:23:50AM +0200, Greg KH wrote:
> > On Fri, May 06, 2022 at 09:23:22AM +0100, Giovanni Cabiddu wrote:
> > > Set to zero the DH context buffers containing the DH key before they are
> > > freed.
> >
> > That says what, but not why.
> >
> > > Cc: [email protected]
> > > Fixes: c9839143ebbf ("crypto: qat - Add DH support")
> > > Signed-off-by: Giovanni Cabiddu <[email protected]>
> > > Reviewed-by: Adam Guerin <[email protected]>
> > > Reviewed-by: Wojciech Ziemba <[email protected]>
> > > ---
> > > drivers/crypto/qat/qat_common/qat_asym_algs.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/crypto/qat/qat_common/qat_asym_algs.c b/drivers/crypto/qat/qat_common/qat_asym_algs.c
> > > index d75eb77c9fb9..2fec89b8a188 100644
> > > --- a/drivers/crypto/qat/qat_common/qat_asym_algs.c
> > > +++ b/drivers/crypto/qat/qat_common/qat_asym_algs.c
> > > @@ -421,14 +421,17 @@ static int qat_dh_set_params(struct qat_dh_ctx *ctx, struct dh *params)
> > > static void qat_dh_clear_ctx(struct device *dev, struct qat_dh_ctx *ctx)
> > > {
> > > if (ctx->g) {
> > > + memzero_explicit(ctx->g, ctx->p_size);
> > > dma_free_coherent(dev, ctx->p_size, ctx->g, ctx->dma_g);
> >
> > Why is a memset() not sufficient here?
> Based on the previous conversation a memset() should be sufficient.
>
> > And what is this solving? Who would get this stale data?
> This is to make sure the buffer containing sensitive data (i.e. a key)
> is not leaked out by a subsequent allocation.
But as all sane distros have CONFIG_INIT_ON_ALLOC_DEFAULT_ON enabled,
right? That should handle any worries you have with secrets being on
the heap. But even then, are you trying to protect yourself against
other kernel modules? Think this through...
thanks,
greg k-h
On Fri, May 06, 2022 at 09:23:22AM +0100, Giovanni Cabiddu wrote:
> Set to zero the DH context buffers containing the DH key before they are
> freed.
That says what, but not why.
> Cc: [email protected]
> Fixes: c9839143ebbf ("crypto: qat - Add DH support")
> Signed-off-by: Giovanni Cabiddu <[email protected]>
> Reviewed-by: Adam Guerin <[email protected]>
> Reviewed-by: Wojciech Ziemba <[email protected]>
> ---
> drivers/crypto/qat/qat_common/qat_asym_algs.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/crypto/qat/qat_common/qat_asym_algs.c b/drivers/crypto/qat/qat_common/qat_asym_algs.c
> index d75eb77c9fb9..2fec89b8a188 100644
> --- a/drivers/crypto/qat/qat_common/qat_asym_algs.c
> +++ b/drivers/crypto/qat/qat_common/qat_asym_algs.c
> @@ -421,14 +421,17 @@ static int qat_dh_set_params(struct qat_dh_ctx *ctx, struct dh *params)
> static void qat_dh_clear_ctx(struct device *dev, struct qat_dh_ctx *ctx)
> {
> if (ctx->g) {
> + memzero_explicit(ctx->g, ctx->p_size);
> dma_free_coherent(dev, ctx->p_size, ctx->g, ctx->dma_g);
Why is a memset() not sufficient here?
And what is this solving? Who would get this stale data?
thanks,
greg k-h
On Fri, May 06, 2022 at 11:23:50AM +0200, Greg KH wrote:
> On Fri, May 06, 2022 at 09:23:22AM +0100, Giovanni Cabiddu wrote:
> > Set to zero the DH context buffers containing the DH key before they are
> > freed.
>
> That says what, but not why.
>
> > Cc: [email protected]
> > Fixes: c9839143ebbf ("crypto: qat - Add DH support")
> > Signed-off-by: Giovanni Cabiddu <[email protected]>
> > Reviewed-by: Adam Guerin <[email protected]>
> > Reviewed-by: Wojciech Ziemba <[email protected]>
> > ---
> > drivers/crypto/qat/qat_common/qat_asym_algs.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/crypto/qat/qat_common/qat_asym_algs.c b/drivers/crypto/qat/qat_common/qat_asym_algs.c
> > index d75eb77c9fb9..2fec89b8a188 100644
> > --- a/drivers/crypto/qat/qat_common/qat_asym_algs.c
> > +++ b/drivers/crypto/qat/qat_common/qat_asym_algs.c
> > @@ -421,14 +421,17 @@ static int qat_dh_set_params(struct qat_dh_ctx *ctx, struct dh *params)
> > static void qat_dh_clear_ctx(struct device *dev, struct qat_dh_ctx *ctx)
> > {
> > if (ctx->g) {
> > + memzero_explicit(ctx->g, ctx->p_size);
> > dma_free_coherent(dev, ctx->p_size, ctx->g, ctx->dma_g);
>
> Why is a memset() not sufficient here?
Based on the previous conversation a memset() should be sufficient.
> And what is this solving? Who would get this stale data?
This is to make sure the buffer containing sensitive data (i.e. a key)
is not leaked out by a subsequent allocation.
I will clarify it in the commit message.
Thanks,
--
Giovanni
On Fri, May 06, 2022 at 04:41:52PM +0200, Greg KH wrote:
> On Fri, May 06, 2022 at 11:01:17AM +0100, Giovanni Cabiddu wrote:
> > On Fri, May 06, 2022 at 11:23:50AM +0200, Greg KH wrote:
> > > On Fri, May 06, 2022 at 09:23:22AM +0100, Giovanni Cabiddu wrote:
> > > > Set to zero the DH context buffers containing the DH key before they are
> > > > freed.
> > >
> > > That says what, but not why.
> > >
> > > > Cc: [email protected]
> > > > Fixes: c9839143ebbf ("crypto: qat - Add DH support")
> > > > Signed-off-by: Giovanni Cabiddu <[email protected]>
> > > > Reviewed-by: Adam Guerin <[email protected]>
> > > > Reviewed-by: Wojciech Ziemba <[email protected]>
> > > > ---
> > > > drivers/crypto/qat/qat_common/qat_asym_algs.c | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/crypto/qat/qat_common/qat_asym_algs.c b/drivers/crypto/qat/qat_common/qat_asym_algs.c
> > > > index d75eb77c9fb9..2fec89b8a188 100644
> > > > --- a/drivers/crypto/qat/qat_common/qat_asym_algs.c
> > > > +++ b/drivers/crypto/qat/qat_common/qat_asym_algs.c
> > > > @@ -421,14 +421,17 @@ static int qat_dh_set_params(struct qat_dh_ctx *ctx, struct dh *params)
> > > > static void qat_dh_clear_ctx(struct device *dev, struct qat_dh_ctx *ctx)
> > > > {
> > > > if (ctx->g) {
> > > > + memzero_explicit(ctx->g, ctx->p_size);
> > > > dma_free_coherent(dev, ctx->p_size, ctx->g, ctx->dma_g);
> > >
> > > Why is a memset() not sufficient here?
> > Based on the previous conversation a memset() should be sufficient.
> >
> > > And what is this solving? Who would get this stale data?
> > This is to make sure the buffer containing sensitive data (i.e. a key)
> > is not leaked out by a subsequent allocation.
>
> But as all sane distros have CONFIG_INIT_ON_ALLOC_DEFAULT_ON enabled,
> right? That should handle any worries you have with secrets being on
> the heap. But even then, are you trying to protect yourself against
> other kernel modules? Think this through...
>
This patch looks fine to me; it's always recommended to zero out crypto keys at
the end of their lifetime so that they can't be recovered from free memory if
system memory is compromised before the memory happens to be allocated and
overwritten again. See the hundreds of existing callers of kfree_sensitive(),
which exist for exactly this reason.
Note that preventing the key from being "leaked out by a subsequent allocation"
is *not* the point, and thus CONFIG_INIT_ON_ALLOC_DEFAULT_ON is irrelevant.
- Eric
On Sat, May 07, 2022 at 11:52:08AM -0700, Eric Biggers wrote:
> On Fri, May 06, 2022 at 04:41:52PM +0200, Greg KH wrote:
> > On Fri, May 06, 2022 at 11:01:17AM +0100, Giovanni Cabiddu wrote:
> > > On Fri, May 06, 2022 at 11:23:50AM +0200, Greg KH wrote:
> > > > On Fri, May 06, 2022 at 09:23:22AM +0100, Giovanni Cabiddu wrote:
> > > > > Set to zero the DH context buffers containing the DH key before they are
> > > > > freed.
> > > >
> > > > That says what, but not why.
> > > >
> > > > > Cc: [email protected]
> > > > > Fixes: c9839143ebbf ("crypto: qat - Add DH support")
> > > > > Signed-off-by: Giovanni Cabiddu <[email protected]>
> > > > > Reviewed-by: Adam Guerin <[email protected]>
> > > > > Reviewed-by: Wojciech Ziemba <[email protected]>
> > > > > ---
> > > > > drivers/crypto/qat/qat_common/qat_asym_algs.c | 3 +++
> > > > > 1 file changed, 3 insertions(+)
> > > > >
> > > > > diff --git a/drivers/crypto/qat/qat_common/qat_asym_algs.c b/drivers/crypto/qat/qat_common/qat_asym_algs.c
> > > > > index d75eb77c9fb9..2fec89b8a188 100644
> > > > > --- a/drivers/crypto/qat/qat_common/qat_asym_algs.c
> > > > > +++ b/drivers/crypto/qat/qat_common/qat_asym_algs.c
> > > > > @@ -421,14 +421,17 @@ static int qat_dh_set_params(struct qat_dh_ctx *ctx, struct dh *params)
> > > > > static void qat_dh_clear_ctx(struct device *dev, struct qat_dh_ctx *ctx)
> > > > > {
> > > > > if (ctx->g) {
> > > > > + memzero_explicit(ctx->g, ctx->p_size);
> > > > > dma_free_coherent(dev, ctx->p_size, ctx->g, ctx->dma_g);
> > > >
> > > > Why is a memset() not sufficient here?
> > > Based on the previous conversation a memset() should be sufficient.
> > >
> > > > And what is this solving? Who would get this stale data?
> > > This is to make sure the buffer containing sensitive data (i.e. a key)
> > > is not leaked out by a subsequent allocation.
> >
> > But as all sane distros have CONFIG_INIT_ON_ALLOC_DEFAULT_ON enabled,
> > right? That should handle any worries you have with secrets being on
> > the heap. But even then, are you trying to protect yourself against
> > other kernel modules? Think this through...
> >
>
> This patch looks fine to me; it's always recommended to zero out crypto keys at
> the end of their lifetime so that they can't be recovered from free memory if
> system memory is compromised before the memory happens to be allocated and
> overwritten again. See the hundreds of existing callers of kfree_sensitive(),
> which exist for exactly this reason.
>
> Note that preventing the key from being "leaked out by a subsequent allocation"
> is *not* the point, and thus CONFIG_INIT_ON_ALLOC_DEFAULT_ON is irrelevant.
I'm going to clarify the commit message and re-send it detached from the
set.
Regards,
--
Giovanni