[PATCH] crypto: talitos: Init zero_entry more compatibly

[PATCH] crypto: talitos: Init zero_entry more compatibly

[Aaron Sierra]

Compiling this driver with my GCC 4.3.1 e500v2 cross-compiler
resulted in a failed build due to the anonymous union/structures
introduced in this commit:

  crypto: talitos - enhanced talitos_desc struct for SEC1

The build error was:

  drivers/crypto/talitos.h:56: error: unknown field 'len' specified in initializer
  drivers/crypto/talitos.h:56: warning: missing braces around initializer
  drivers/crypto/talitos.h:56: warning: (near initialization for 'zero_entry.<anonymous>')
  drivers/crypto/talitos.h:57: error: unknown field 'j_extent' specified in initializer
  drivers/crypto/talitos.h:58: error: unknown field 'eptr' specified in initializer
  drivers/crypto/talitos.h:58: warning: excess elements in struct initializer
  drivers/crypto/talitos.h:58: warning: (near initialization for 'zero_entry')
  make[2]: *** [drivers/crypto/talitos.o] Error 1
  make[1]: *** [drivers/crypto] Error 2
  make: *** [drivers] Error 2

This patch works around the errors by changing the static constant
zero_entry's initializer to use an initialization shorthand.

Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
---
 drivers/crypto/talitos.h | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/crypto/talitos.h b/drivers/crypto/talitos.h
index 314daf5..bf88fe7 100644
--- a/drivers/crypto/talitos.h
+++ b/drivers/crypto/talitos.h
@@ -52,12 +52,7 @@ struct talitos_ptr {
 	__be32 ptr;     /* address */
 };
 
-static const struct talitos_ptr zero_entry = {
-	.len = 0,
-	.j_extent = 0,
-	.eptr = 0,
-	.ptr = 0
-};
+static const struct talitos_ptr zero_entry = {{{ 0 }}};
 
 /* descriptor */
 struct talitos_desc {
-- 
1.9.1

[PATCH] crypto: talitos: Prevent panic in probe error path

[Aaron Sierra]

RNG unregistration and per-channel FIFO cleanup can cause a kernel
panic, depending on how early in talitos_probe() an error occurs.
This patch prevents these panics from happening.

For completeness, this patch also sets device drvdata to NULL after
the private structure is freed in talitos_remove().

Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
---
 drivers/crypto/talitos.c | 33 +++++++++++++++++++++++----------
 drivers/crypto/talitos.h |  2 +-
 2 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 83aca95..684fe89 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -766,21 +766,33 @@ static int talitos_rng_init(struct hwrng *rng)
 static int talitos_register_rng(struct device *dev)
 {
 	struct talitos_private *priv = dev_get_drvdata(dev);
+	int err = 0;
 
-	priv->rng.name		= dev_driver_string(dev),
-	priv->rng.init		= talitos_rng_init,
-	priv->rng.data_present	= talitos_rng_data_present,
-	priv->rng.data_read	= talitos_rng_data_read,
-	priv->rng.priv		= (unsigned long)dev;
+	priv->rng = kzalloc(sizeof(struct hwrng), GFP_KERNEL);
+	if (!priv->rng)
+		return -ENOMEM;
+
+	priv->rng->name		= dev_driver_string(dev),
+	priv->rng->init		= talitos_rng_init,
+	priv->rng->data_present	= talitos_rng_data_present,
+	priv->rng->data_read	= talitos_rng_data_read,
+	priv->rng->priv		= (unsigned long)dev;
+
+	err = hwrng_register(priv->rng);
+	if (err) {
+		kfree(priv->rng);
+		priv->rng = NULL;
+	}
 
-	return hwrng_register(&priv->rng);
+	return err;
 }
 
 static void talitos_unregister_rng(struct device *dev)
 {
 	struct talitos_private *priv = dev_get_drvdata(dev);
 
-	hwrng_unregister(&priv->rng);
+	if (priv->rng)
+		hwrng_unregister(priv->rng);
 }
 
 /*
@@ -2727,7 +2739,7 @@ static int talitos_remove(struct platform_device *ofdev)
 	if (hw_supports(dev, DESC_HDR_SEL0_RNG))
 		talitos_unregister_rng(dev);
 
-	for (i = 0; i < priv->num_channels; i++)
+	for (i = 0; priv->chan && i < priv->num_channels; i++)
 		kfree(priv->chan[i].fifo);
 
 	kfree(priv->chan);
@@ -2746,6 +2758,8 @@ static int talitos_remove(struct platform_device *ofdev)
 
 	kfree(priv);
 
+	dev_set_drvdata(dev, NULL);
+
 	return 0;
 }
 
@@ -2905,8 +2919,7 @@ static int talitos_probe(struct platform_device *ofdev)
 	priv->reg = of_iomap(np, 0);
 	if (!priv->reg) {
 		dev_err(dev, "failed to of_iomap\n");
-		err = -ENOMEM;
-		goto err_out;
+		return -ENOMEM;
 	}
 
 	/* get SEC version capabilities from device tree */
diff --git a/drivers/crypto/talitos.h b/drivers/crypto/talitos.h
index bf88fe7..e86f67c 100644
--- a/drivers/crypto/talitos.h
+++ b/drivers/crypto/talitos.h
@@ -148,7 +148,7 @@ struct talitos_private {
 	struct list_head alg_list;
 
 	/* hwrng device */
-	struct hwrng rng;
+	struct hwrng *rng;
 };
 
 extern int talitos_submit(struct device *dev, int ch, struct talitos_desc *desc,
-- 
1.9.1

Re: [PATCH] crypto: talitos: Init zero_entry more compatibly

[Herbert Xu]

On Fri, Jul 31, 2015 at 12:27:41PM -0500, Aaron Sierra wrote:
>
> -static const struct talitos_ptr zero_entry = {
> -	.len = 0,
> -	.j_extent = 0,
> -	.eptr = 0,
> -	.ptr = 0
> -};
> +static const struct talitos_ptr zero_entry = {{{ 0 }}};

Please get rid of the initialiser completely.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH v2] crypto: talitos: Remove zero_entry static initializer

[Aaron Sierra]

Compiling the talitos driver with my GCC 4.3.1 e500v2 cross-compiler
resulted in a failed build due to the anonymous union/structures
introduced in this commit:

  crypto: talitos - enhanced talitos_desc struct for SEC1

The build error was:

  drivers/crypto/talitos.h:56: error: unknown field 'len' specified in initializer
  drivers/crypto/talitos.h:56: warning: missing braces around initializer
  drivers/crypto/talitos.h:56: warning: (near initialization for 'zero_entry.<anonymous>')
  drivers/crypto/talitos.h:57: error: unknown field 'j_extent' specified in initializer
  drivers/crypto/talitos.h:58: error: unknown field 'eptr' specified in initializer
  drivers/crypto/talitos.h:58: warning: excess elements in struct initializer
  drivers/crypto/talitos.h:58: warning: (near initialization for 'zero_entry')
  make[2]: *** [drivers/crypto/talitos.o] Error 1
  make[1]: *** [drivers/crypto] Error 2
  make: *** [drivers] Error 2

This patch eliminates the errors by moving the static constant
zero_entry to the talitos_private structure. As a member of that
structure, zero_entry gets initialized for free (and compatibly) by
the kzalloc() during device probe.

Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
---
 drivers/crypto/talitos.c | 14 +++++++-------
 drivers/crypto/talitos.h |  8 +-------
 2 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 83aca95..5347570 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -1663,7 +1663,7 @@ static int common_nonsnoop(struct talitos_edesc *edesc,
 	bool is_sec1 = has_ftr_sec1(priv);
 
 	/* first DWORD empty */
-	desc->ptr[0] = zero_entry;
+	desc->ptr[0] = priv->zero_entry;
 
 	/* cipher iv */
 	to_talitos_ptr(&desc->ptr[1], edesc->iv_dma, is_sec1);
@@ -1693,7 +1693,7 @@ static int common_nonsnoop(struct talitos_edesc *edesc,
 			       DMA_FROM_DEVICE);
 
 	/* last DWORD empty */
-	desc->ptr[6] = zero_entry;
+	desc->ptr[6] = priv->zero_entry;
 
 	ret = talitos_submit(dev, ctx->ch, desc, callback, areq);
 	if (ret != -EINPROGRESS) {
@@ -1833,7 +1833,7 @@ static int common_nonsnoop_hash(struct talitos_edesc *edesc,
 	bool is_sec1 = has_ftr_sec1(priv);
 
 	/* first DWORD empty */
-	desc->ptr[0] = zero_entry;
+	desc->ptr[0] = priv->zero_entry;
 
 	/* hash context in */
 	if (!req_ctx->first || req_ctx->swinit) {
@@ -1843,7 +1843,7 @@ static int common_nonsnoop_hash(struct talitos_edesc *edesc,
 				       DMA_TO_DEVICE);
 		req_ctx->swinit = 0;
 	} else {
-		desc->ptr[1] = zero_entry;
+		desc->ptr[1] = priv->zero_entry;
 		/* Indicate next op is not the first. */
 		req_ctx->first = 0;
 	}
@@ -1853,7 +1853,7 @@ static int common_nonsnoop_hash(struct talitos_edesc *edesc,
 		map_single_talitos_ptr(dev, &desc->ptr[2], ctx->keylen,
 				       (char *)&ctx->key, DMA_TO_DEVICE);
 	else
-		desc->ptr[2] = zero_entry;
+		desc->ptr[2] = priv->zero_entry;
 
 	/*
 	 * data in
@@ -1862,7 +1862,7 @@ static int common_nonsnoop_hash(struct talitos_edesc *edesc,
 			      DMA_TO_DEVICE, &desc->ptr[3]);
 
 	/* fifth DWORD empty */
-	desc->ptr[4] = zero_entry;
+	desc->ptr[4] = priv->zero_entry;
 
 	/* hash/HMAC out -or- hash context out */
 	if (req_ctx->last)
@@ -1875,7 +1875,7 @@ static int common_nonsnoop_hash(struct talitos_edesc *edesc,
 				       req_ctx->hw_context, DMA_FROM_DEVICE);
 
 	/* last DWORD empty */
-	desc->ptr[6] = zero_entry;
+	desc->ptr[6] = priv->zero_entry;
 
 	if (is_sec1 && from_talitos_ptr_len(&desc->ptr[3], true) == 0)
 		talitos_handle_buggy_hash(ctx, edesc, &desc->ptr[3]);
diff --git a/drivers/crypto/talitos.h b/drivers/crypto/talitos.h
index 314daf5..153c56a 100644
--- a/drivers/crypto/talitos.h
+++ b/drivers/crypto/talitos.h
@@ -52,13 +52,6 @@ struct talitos_ptr {
 	__be32 ptr;     /* address */
 };
 
-static const struct talitos_ptr zero_entry = {
-	.len = 0,
-	.j_extent = 0,
-	.eptr = 0,
-	.ptr = 0
-};
-
 /* descriptor */
 struct talitos_desc {
 	__be32 hdr;                     /* header high bits */
@@ -142,6 +135,7 @@ struct talitos_private {
 	unsigned int fifo_len;
 
 	struct talitos_channel *chan;
+	const struct talitos_ptr zero_entry;
 
 	/* next channel to be assigned next incoming descriptor */
 	atomic_t last_chan ____cacheline_aligned;
-- 
1.9.1

Re: [PATCH v2] crypto: talitos: Remove zero_entry static initializer

[Herbert Xu]

On Mon, Aug 03, 2015 at 11:14:37AM -0500, Aaron Sierra wrote:
> Compiling the talitos driver with my GCC 4.3.1 e500v2 cross-compiler
> resulted in a failed build due to the anonymous union/structures
> introduced in this commit:
> 
>   crypto: talitos - enhanced talitos_desc struct for SEC1
> 
> The build error was:
> 
>   drivers/crypto/talitos.h:56: error: unknown field 'len' specified in initializer
>   drivers/crypto/talitos.h:56: warning: missing braces around initializer
>   drivers/crypto/talitos.h:56: warning: (near initialization for 'zero_entry.<anonymous>')
>   drivers/crypto/talitos.h:57: error: unknown field 'j_extent' specified in initializer
>   drivers/crypto/talitos.h:58: error: unknown field 'eptr' specified in initializer
>   drivers/crypto/talitos.h:58: warning: excess elements in struct initializer
>   drivers/crypto/talitos.h:58: warning: (near initialization for 'zero_entry')
>   make[2]: *** [drivers/crypto/talitos.o] Error 1
>   make[1]: *** [drivers/crypto] Error 2
>   make: *** [drivers] Error 2
> 
> This patch eliminates the errors by moving the static constant
> zero_entry to the talitos_private structure. As a member of that
> structure, zero_entry gets initialized for free (and compatibly) by
> the kzalloc() during device probe.

Why? A static variable is always zeroed.  Please just get rid
of the initialisation and it will work.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2] crypto: talitos: Remove zero_entry static initializer

[Aaron Sierra]

----- Original Message -----
> From: "Herbert Xu" <herbert@gondor.apana.org.au>
> Sent: Monday, August 3, 2015 6:31:20 PM
> 
> On Mon, Aug 03, 2015 at 11:14:37AM -0500, Aaron Sierra wrote:
> > Compiling the talitos driver with my GCC 4.3.1 e500v2 cross-compiler
> > resulted in a failed build due to the anonymous union/structures
> > introduced in this commit:
> > 
> >   crypto: talitos - enhanced talitos_desc struct for SEC1
> > 
> > The build error was:
> > 
> >   drivers/crypto/talitos.h:56: error: unknown field 'len' specified in
> >   initializer
> >   drivers/crypto/talitos.h:56: warning: missing braces around initializer
> >   drivers/crypto/talitos.h:56: warning: (near initialization for
> >   'zero_entry.<anonymous>')
> >   drivers/crypto/talitos.h:57: error: unknown field 'j_extent' specified in
> >   initializer
> >   drivers/crypto/talitos.h:58: error: unknown field 'eptr' specified in
> >   initializer
> >   drivers/crypto/talitos.h:58: warning: excess elements in struct
> >   initializer
> >   drivers/crypto/talitos.h:58: warning: (near initialization for
> >   'zero_entry')
> >   make[2]: *** [drivers/crypto/talitos.o] Error 1
> >   make[1]: *** [drivers/crypto] Error 2
> >   make: *** [drivers] Error 2
> > 
> > This patch eliminates the errors by moving the static constant
> > zero_entry to the talitos_private structure. As a member of that
> > structure, zero_entry gets initialized for free (and compatibly) by
> > the kzalloc() during device probe.
> 
> Why? A static variable is always zeroed.  Please just get rid
> of the initialisation and it will work.

Thanks for elaborating. I'll make the change.

-Aaron

[PATCH v3] crypto: talitos: Remove zero_entry static initializer

[Aaron Sierra]

Compiling the talitos driver with my GCC 4.3.1 e500v2 cross-compiler
resulted in a failed build due to the anonymous union/structures
introduced in this commit:

  crypto: talitos - enhanced talitos_desc struct for SEC1

The build error was:

  drivers/crypto/talitos.h:56: error: unknown field 'len' specified in initializer
  drivers/crypto/talitos.h:56: warning: missing braces around initializer
  drivers/crypto/talitos.h:56: warning: (near initialization for 'zero_entry.<anonymous>')
  drivers/crypto/talitos.h:57: error: unknown field 'j_extent' specified in initializer
  drivers/crypto/talitos.h:58: error: unknown field 'eptr' specified in initializer
  drivers/crypto/talitos.h:58: warning: excess elements in struct initializer
  drivers/crypto/talitos.h:58: warning: (near initialization for 'zero_entry')
  make[2]: *** [drivers/crypto/talitos.o] Error 1
  make[1]: *** [drivers/crypto] Error 2
  make: *** [drivers] Error 2

This patch eliminates the errors by relying on the C standard's
implicit assignment of zero to static variables.

Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
---
 drivers/crypto/talitos.h | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/crypto/talitos.h b/drivers/crypto/talitos.h
index 314daf5..163cfe7 100644
--- a/drivers/crypto/talitos.h
+++ b/drivers/crypto/talitos.h
@@ -52,12 +52,7 @@ struct talitos_ptr {
 	__be32 ptr;     /* address */
 };
 
-static const struct talitos_ptr zero_entry = {
-	.len = 0,
-	.j_extent = 0,
-	.eptr = 0,
-	.ptr = 0
-};
+static const struct talitos_ptr zero_entry;
 
 /* descriptor */
 struct talitos_desc {
-- 
1.9.1

Re: [PATCH] crypto: talitos: Prevent panic in probe error path

[Herbert Xu]

On Fri, Jul 31, 2015 at 03:52:18PM -0500, Aaron Sierra wrote:
>
> @@ -2905,8 +2919,7 @@ static int talitos_probe(struct platform_device *ofdev)
>  	priv->reg = of_iomap(np, 0);
>  	if (!priv->reg) {
>  		dev_err(dev, "failed to of_iomap\n");
> -		err = -ENOMEM;
> -		goto err_out;
> +		return -ENOMEM;
>  	}

This is the only bit that seems remotely related to your change
description.  Please ensure that your patch actually corresponds
to your changelog and do not include unrelated changes without
documenting them.

And even this bit is wrong because you're leaking priv.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v3] crypto: talitos: Remove zero_entry static initializer

[Herbert Xu]

On Mon, Aug 03, 2015 at 06:56:21PM -0500, Aaron Sierra wrote:
> Compiling the talitos driver with my GCC 4.3.1 e500v2 cross-compiler
> resulted in a failed build due to the anonymous union/structures
> introduced in this commit:
> 
>   crypto: talitos - enhanced talitos_desc struct for SEC1
> 
> The build error was:
> 
>   drivers/crypto/talitos.h:56: error: unknown field 'len' specified in initializer
>   drivers/crypto/talitos.h:56: warning: missing braces around initializer
>   drivers/crypto/talitos.h:56: warning: (near initialization for 'zero_entry.<anonymous>')
>   drivers/crypto/talitos.h:57: error: unknown field 'j_extent' specified in initializer
>   drivers/crypto/talitos.h:58: error: unknown field 'eptr' specified in initializer
>   drivers/crypto/talitos.h:58: warning: excess elements in struct initializer
>   drivers/crypto/talitos.h:58: warning: (near initialization for 'zero_entry')
>   make[2]: *** [drivers/crypto/talitos.o] Error 1
>   make[1]: *** [drivers/crypto] Error 2
>   make: *** [drivers] Error 2
> 
> This patch eliminates the errors by relying on the C standard's
> implicit assignment of zero to static variables.
> 
> Signed-off-by: Aaron Sierra <asierra@xes-inc.com>

Applied.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: talitos: Prevent panic in probe error path

[Aaron Sierra]

----- Original Message -----
> From: "Herbert Xu" <herbert@gondor.apana.org.au>
> Sent: Tuesday, August 4, 2015 2:18:05 AM
> 
> On Fri, Jul 31, 2015 at 03:52:18PM -0500, Aaron Sierra wrote:
> >
> > @@ -2905,8 +2919,7 @@ static int talitos_probe(struct platform_device
> > *ofdev)
> >  	priv->reg = of_iomap(np, 0);
> >  	if (!priv->reg) {
> >  		dev_err(dev, "failed to of_iomap\n");
> > -		err = -ENOMEM;
> > -		goto err_out;
> > +		return -ENOMEM;
> >  	}
> 
> This is the only bit that seems remotely related to your change
> description.  Please ensure that your patch actually corresponds
> to your changelog and do not include unrelated changes without
> documenting them.
> 
> And even this bit is wrong because you're leaking priv.

Herbert,

You are correct about the leak and I regret introducing that (I am
also leaking priv->rng), but I disagree with your dismissal of the
rest of the changes as unrelated to the changelog.

The probe error path for this driver, for all intents and purposes,
is the talitos_remove() function due to the common "goto err_out".

Without my patch applied, talitos_remove() will panic under the
two conditions that I outlined in the changelog:

1. If the RNG device hasn't been registered via
   talitos_register_rng() prior to entry into talitos_remove(),
   then the attempt to unregister the RNG "device" will cause a panic.

2. If the priv->chan array has not been allocated prior to entry
   into talitos_remove(), then the per-channel FIFO cleanup will panic
   because of the dereference of that NULL "array".

Both of the above scenarios occur if talitos_probe_irq() fails.

The patch that I submitted resolves issue #1 by changing priv->rng
to a struct hwrng pointer, which allows talitos_unregister_rng() to
know whether an RNG device has been registered (or at least prepared
for registration). As an alternative, I considered introducing a
boolean to serve the same purpose.

It resolves issue #2 by checking that priv->chan is not NULL in the
per-channel FIFO cleanup for loop.

-Aaron

Re: [PATCH] crypto: talitos: Prevent panic in probe error path

[Herbert Xu]

On Tue, Aug 04, 2015 at 09:43:50AM -0500, Aaron Sierra wrote:
> 
> You are correct about the leak and I regret introducing that (I am
> also leaking priv->rng), but I disagree with your dismissal of the
> rest of the changes as unrelated to the changelog.

I understand the problem, but your change description is way
too vague and does not correspond to the change, especially the
bit where you're replacing the static variable with kzalloc.  You
should have included the following text in your description.

> The probe error path for this driver, for all intents and purposes,
> is the talitos_remove() function due to the common "goto err_out".
> 
> Without my patch applied, talitos_remove() will panic under the
> two conditions that I outlined in the changelog:
> 
> 1. If the RNG device hasn't been registered via
>    talitos_register_rng() prior to entry into talitos_remove(),
>    then the attempt to unregister the RNG "device" will cause a panic.
> 
> 2. If the priv->chan array has not been allocated prior to entry
>    into talitos_remove(), then the per-channel FIFO cleanup will panic
>    because of the dereference of that NULL "array".
> 
> Both of the above scenarios occur if talitos_probe_irq() fails.
> 
> The patch that I submitted resolves issue #1 by changing priv->rng
> to a struct hwrng pointer, which allows talitos_unregister_rng() to
> know whether an RNG device has been registered (or at least prepared
> for registration). As an alternative, I considered introducing a
> boolean to serve the same purpose.

I would prefer a boolean as that means less churn.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: talitos: Prevent panic in probe error path

[Aaron Sierra]

----- Original Message -----
> From: "Herbert Xu" <herbert@gondor.apana.org.au>
> Sent: Tuesday, August 4, 2015 7:08:13 PM
> 
> On Tue, Aug 04, 2015 at 09:43:50AM -0500, Aaron Sierra wrote:
> > 
> > You are correct about the leak and I regret introducing that (I am
> > also leaking priv->rng), but I disagree with your dismissal of the
> > rest of the changes as unrelated to the changelog.
> 
> I understand the problem, but your change description is way
> too vague and does not correspond to the change, especially the
> bit where you're replacing the static variable with kzalloc.  You
> should have included the following text in your description.
> 
> > The probe error path for this driver, for all intents and purposes,
> > is the talitos_remove() function due to the common "goto err_out".
> > 
> > Without my patch applied, talitos_remove() will panic under the
> > two conditions that I outlined in the changelog:
> > 
> > 1. If the RNG device hasn't been registered via
> >    talitos_register_rng() prior to entry into talitos_remove(),
> >    then the attempt to unregister the RNG "device" will cause a panic.
> > 
> > 2. If the priv->chan array has not been allocated prior to entry
> >    into talitos_remove(), then the per-channel FIFO cleanup will panic
> >    because of the dereference of that NULL "array".
> > 
> > Both of the above scenarios occur if talitos_probe_irq() fails.
> > 
> > The patch that I submitted resolves issue #1 by changing priv->rng
> > to a struct hwrng pointer, which allows talitos_unregister_rng() to
> > know whether an RNG device has been registered (or at least prepared
> > for registration). As an alternative, I considered introducing a
> > boolean to serve the same purpose.
> 
> I would prefer a boolean as that means less churn.
> 

Herbert,
Thanks for the feedback. I'll address your concerns in the next
version.

-Aaron

[PATCH v2] crypto: talitos: Prevent panic in probe error path

[Aaron Sierra]

The probe error path for this driver, for all intents and purposes,
is the talitos_remove() function due to the common "goto err_out".

Without this patch applied, talitos_remove() will panic under these
two conditions:

1. If the RNG device hasn't been registered via
   talitos_register_rng() prior to entry into talitos_remove(),
   then the attempt to unregister the RNG "device" will cause a panic.

2. If the priv->chan array has not been allocated prior to entry
   into talitos_remove(), then the per-channel FIFO cleanup will panic
   because of the dereference of that NULL "array".

Both of the above scenarios occur if talitos_probe_irq() fails.

This patch resolves issue #1 by introducing a boolean to mask the
hwrng_unregister() call in talitos_unregister_rng() if RNG device
registration was unsuccessful.

It resolves issue #2 by checking that priv->chan is not NULL in the
per-channel FIFO cleanup for loop.

For completeness, this patch also sets device drvdata to NULL after
the private structure is freed in talitos_remove().

Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
---
 drivers/crypto/talitos.c | 14 ++++++++++++--
 drivers/crypto/talitos.h |  1 +
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 83aca95..74f1a62 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -766,6 +766,7 @@ static int talitos_rng_init(struct hwrng *rng)
 static int talitos_register_rng(struct device *dev)
 {
 	struct talitos_private *priv = dev_get_drvdata(dev);
+	int err;
 
 	priv->rng.name		= dev_driver_string(dev),
 	priv->rng.init		= talitos_rng_init,
@@ -773,14 +774,22 @@ static int talitos_register_rng(struct device *dev)
 	priv->rng.data_read	= talitos_rng_data_read,
 	priv->rng.priv		= (unsigned long)dev;
 
-	return hwrng_register(&priv->rng);
+	err = hwrng_register(&priv->rng);
+	if (!err)
+		priv->rng_registered = true;
+
+	return err;
 }
 
 static void talitos_unregister_rng(struct device *dev)
 {
 	struct talitos_private *priv = dev_get_drvdata(dev);
 
+	if (!priv->rng_registered)
+		return;
+
 	hwrng_unregister(&priv->rng);
+	priv->rng_registered = false;
 }
 
 /*
@@ -2727,7 +2736,7 @@ static int talitos_remove(struct platform_device *ofdev)
 	if (hw_supports(dev, DESC_HDR_SEL0_RNG))
 		talitos_unregister_rng(dev);
 
-	for (i = 0; i < priv->num_channels; i++)
+	for (i = 0; priv->chan && i < priv->num_channels; i++)
 		kfree(priv->chan[i].fifo);
 
 	kfree(priv->chan);
@@ -2745,6 +2754,7 @@ static int talitos_remove(struct platform_device *ofdev)
 	iounmap(priv->reg);
 
 	kfree(priv);
+	dev_set_drvdata(dev, NULL);
 
 	return 0;
 }
diff --git a/drivers/crypto/talitos.h b/drivers/crypto/talitos.h
index 163cfe7..0090f32 100644
--- a/drivers/crypto/talitos.h
+++ b/drivers/crypto/talitos.h
@@ -149,6 +149,7 @@ struct talitos_private {
 
 	/* hwrng device */
 	struct hwrng rng;
+	bool rng_registered;
 };
 
 extern int talitos_submit(struct device *dev, int ch, struct talitos_desc *desc,
-- 
1.9.1

Re: [PATCH v2] crypto: talitos: Prevent panic in probe error path

[Aaron Sierra]

> For completeness, this patch also sets device drvdata to NULL after
> the private structure is freed in talitos_remove().

[snip]

> @@ -2745,6 +2754,7 @@ static int talitos_remove(struct platform_device
> *ofdev)
>  	iounmap(priv->reg);
>  
>  	kfree(priv);
> +	dev_set_drvdata(dev, NULL);
>  
>  	return 0;
>  }

I just found this drvdata cleanup hasn't been required for a few years,
due to the following commit:

    commit 0998d0631001288a5974afc0b2a5f568bcdecb4d
    Author: Hans de Goede <hdegoede@redhat.com>
    Date:   Wed May 23 00:09:34 2012 +0200

        device-core: Ensure drvdata = NULL when no driver is bound

Should I submit a v3 patch without this cleanup, assuming there aren't
other changes requested?

-Aaron

Re: [PATCH v2] crypto: talitos: Prevent panic in probe error path

[Herbert Xu]

On Thu, Aug 06, 2015 at 01:45:42PM -0500, Aaron Sierra wrote:
>
> I just found this drvdata cleanup hasn't been required for a few years,
> due to the following commit:
> 
>     commit 0998d0631001288a5974afc0b2a5f568bcdecb4d
>     Author: Hans de Goede <hdegoede@redhat.com>
>     Date:   Wed May 23 00:09:34 2012 +0200
> 
>         device-core: Ensure drvdata = NULL when no driver is bound
> 
> Should I submit a v3 patch without this cleanup, assuming there aren't
> other changes requested?

I have applied your patch with the dev_set_drvdata change removed.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt