[PATCH 0/4] crypto: s5p-sss - Fix and minor improvements

[PATCH 0/4] crypto: s5p-sss - Fix and minor improvements

[Krzysztof Kozlowski]

Hi,

I still did not fix the NULL pointer dereference reported by
Nathan Royce [1], but I got some other improvements.

Testing done on Odroid U3 (Exynos4412) with tcrypt and cryptsetup.

Best regards,
Krzysztof


[1] https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1351149.html

Krzysztof Kozlowski (4):
  crypto: s5p-sss - Close possible race for completed requests
  crypto: s5p-sss - Remove unused variant field from state container
  crypto: s5p-sss - Document the struct s5p_aes_dev
  crypto: s5p-sss - Use mutex instead of spinlock

 drivers/crypto/s5p-sss.c | 70 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 45 insertions(+), 25 deletions(-)

-- 
2.9.3

[PATCH 1/4] crypto: s5p-sss - Close possible race for completed requests

[Krzysztof Kozlowski]

Driver is capable of handling only one request at a time and it stores
it in its state container struct s5p_aes_dev.  This stored request must be
protected between concurrent invocations (e.g. completing current
request and scheduling new one).  Combination of lock and "busy" field
is used for that purpose.

When "busy" field is true, the driver will not accept new request thus
it will not overwrite currently handled data.

However commit 28b62b145868 ("crypto: s5p-sss - Fix spinlock recursion
on LRW(AES)") moved some of the write to "busy" field out of a lock
protected critical section.  This might lead to potential race between
completing current request and scheduling a new one.  Effectively the
request completion might try to operate on new crypto request.

Cc: <stable@vger.kernel.org> # v4.10.x
Fixes: 28b62b145868 ("crypto: s5p-sss - Fix spinlock recursion on LRW(AES)")
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/crypto/s5p-sss.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
index 1b9da3dc799b..6c620487e9c2 100644
--- a/drivers/crypto/s5p-sss.c
+++ b/drivers/crypto/s5p-sss.c
@@ -287,7 +287,6 @@ static void s5p_sg_done(struct s5p_aes_dev *dev)
 static void s5p_aes_complete(struct s5p_aes_dev *dev, int err)
 {
 	dev->req->base.complete(&dev->req->base, err);
-	dev->busy = false;
 }
 
 static void s5p_unset_outdata(struct s5p_aes_dev *dev)
@@ -462,7 +461,7 @@ static irqreturn_t s5p_aes_interrupt(int irq, void *dev_id)
 		spin_unlock_irqrestore(&dev->lock, flags);
 
 		s5p_aes_complete(dev, 0);
-		dev->busy = true;
+		/* Device is still busy */
 		tasklet_schedule(&dev->tasklet);
 	} else {
 		/*
@@ -483,6 +482,7 @@ static irqreturn_t s5p_aes_interrupt(int irq, void *dev_id)
 
 error:
 	s5p_sg_done(dev);
+	dev->busy = false;
 	spin_unlock_irqrestore(&dev->lock, flags);
 	s5p_aes_complete(dev, err);
 
@@ -634,6 +634,7 @@ static void s5p_aes_crypt_start(struct s5p_aes_dev *dev, unsigned long mode)
 
 indata_error:
 	s5p_sg_done(dev);
+	dev->busy = false;
 	spin_unlock_irqrestore(&dev->lock, flags);
 	s5p_aes_complete(dev, err);
 }
-- 
2.9.3

Re: [PATCH 1/4] crypto: s5p-sss - Close possible race for completed requests

[Bartlomiej Zolnierkiewicz]

On Friday, March 17, 2017 04:49:19 PM Krzysztof Kozlowski wrote:
> Driver is capable of handling only one request at a time and it stores
> it in its state container struct s5p_aes_dev.  This stored request must be
> protected between concurrent invocations (e.g. completing current
> request and scheduling new one).  Combination of lock and "busy" field
> is used for that purpose.
> 
> When "busy" field is true, the driver will not accept new request thus
> it will not overwrite currently handled data.
> 
> However commit 28b62b145868 ("crypto: s5p-sss - Fix spinlock recursion
> on LRW(AES)") moved some of the write to "busy" field out of a lock
> protected critical section.  This might lead to potential race between
> completing current request and scheduling a new one.  Effectively the
> request completion might try to operate on new crypto request.
> 
> Cc: <stable@vger.kernel.org> # v4.10.x
> Fixes: 28b62b145868 ("crypto: s5p-sss - Fix spinlock recursion on LRW(AES)")
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>

Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Best regards,

Re: [PATCH 1/4] crypto: s5p-sss - Close possible race for completed requests

[Bartlomiej Zolnierkiewicz]

On Friday, March 17, 2017 04:49:19 PM Krzysztof Kozlowski wrote:
> Driver is capable of handling only one request at a time and it stores
> it in its state container struct s5p_aes_dev.  This stored request must be
> protected between concurrent invocations (e.g. completing current
> request and scheduling new one).  Combination of lock and "busy" field
> is used for that purpose.
> 
> When "busy" field is true, the driver will not accept new request thus
> it will not overwrite currently handled data.
> 
> However commit 28b62b145868 ("crypto: s5p-sss - Fix spinlock recursion
> on LRW(AES)") moved some of the write to "busy" field out of a lock
> protected critical section.  This might lead to potential race between
> completing current request and scheduling a new one.  Effectively the
> request completion might try to operate on new crypto request.
> 
> Cc: <stable@vger.kernel.org> # v4.10.x
> Fixes: 28b62b145868 ("crypto: s5p-sss - Fix spinlock recursion on LRW(AES)")
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>

Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

[PATCH 2/4] crypto: s5p-sss - Remove unused variant field from state container

[Krzysztof Kozlowski]

The driver uses type of device (variant) only during probe so there is
no need to store it for later.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/crypto/s5p-sss.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
index 6c620487e9c2..35ea84b7d775 100644
--- a/drivers/crypto/s5p-sss.c
+++ b/drivers/crypto/s5p-sss.c
@@ -190,8 +190,6 @@ struct s5p_aes_dev {
 	struct crypto_queue		queue;
 	bool				busy;
 	spinlock_t			lock;
-
-	struct samsung_aes_variant	*variant;
 };
 
 static struct s5p_aes_dev *s5p_dev;
@@ -852,7 +850,6 @@ static int s5p_aes_probe(struct platform_device *pdev)
 	}
 
 	pdata->busy = false;
-	pdata->variant = variant;
 	pdata->dev = dev;
 	platform_set_drvdata(pdev, pdata);
 	s5p_dev = pdata;
-- 
2.9.3

Re: [PATCH 2/4] crypto: s5p-sss - Remove unused variant field from state container

[Bartlomiej Zolnierkiewicz]

On Friday, March 17, 2017 04:49:20 PM Krzysztof Kozlowski wrote:
> The driver uses type of device (variant) only during probe so there is
> no need to store it for later.
> 
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>

Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

[PATCH 3/4] crypto: s5p-sss - Document the struct s5p_aes_dev

[Krzysztof Kozlowski]

Add kernel-doc to s5p_aes_dev structure.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/crypto/s5p-sss.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
index 35ea84b7d775..7ac657f46d15 100644
--- a/drivers/crypto/s5p-sss.c
+++ b/drivers/crypto/s5p-sss.c
@@ -170,6 +170,32 @@ struct s5p_aes_ctx {
 	int				keylen;
 };
 
+/**
+ * struct s5p_aes_dev - Crypto device state container
+ * @dev:	Associated device
+ * @clk:	Clock for accessing hardware
+ * @ioaddr:	Mapped IO memory region
+ * @aes_ioaddr:	Per-varian offset for AES block IO memory
+ * @irq_fc:	Feed control interrupt line
+ * @req:	Crypto request currently handled by the device
+ * @ctx:	Configuration for currently handled crypto request
+ * @sg_src:	Scatter list with source data for currently handled block
+ *		in device.  This is DMA-mapped into device.
+ * @sg_dst:	Scatter list with destination data for currently handled block
+ *		in device. This is DMA-mapped into device.
+ * @sg_src_cpy:	In case of unaligned access, copied scatter list
+ *		with source data.
+ * @sg_dst_cpy:	In case of unaligned access, copied scatter list
+ *		with destination data.
+ * @tasklet:	New request scheduling jib
+ * @queue:	Crypto queue
+ * @busy:	Indicates whether the device is currently handling some request
+ *		thus it uses some of the fields from this state, like:
+ *		req, ctx, sg_src/dst (and copies).  This essentially
+ *		protects against concurrent access to these fields.
+ * @lock:	Lock for protecting both access to device hardware registers
+ *		and fields related to current request (including the busy field).
+ */
 struct s5p_aes_dev {
 	struct device			*dev;
 	struct clk			*clk;
@@ -182,7 +208,6 @@ struct s5p_aes_dev {
 	struct scatterlist		*sg_src;
 	struct scatterlist		*sg_dst;
 
-	/* In case of unaligned access: */
 	struct scatterlist		*sg_src_cpy;
 	struct scatterlist		*sg_dst_cpy;
 
-- 
2.9.3

Re: [PATCH 3/4] crypto: s5p-sss - Document the struct s5p_aes_dev

[Bartlomiej Zolnierkiewicz]

On Friday, March 17, 2017 04:49:21 PM Krzysztof Kozlowski wrote:
> Add kernel-doc to s5p_aes_dev structure.
> 
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>

Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Best regards,

Re: [PATCH 3/4] crypto: s5p-sss - Document the struct s5p_aes_dev

[Bartlomiej Zolnierkiewicz]

On Friday, March 17, 2017 04:49:21 PM Krzysztof Kozlowski wrote:
> Add kernel-doc to s5p_aes_dev structure.
> 
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>

Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

[PATCH 4/4] crypto: s5p-sss - Use mutex instead of spinlock

[Krzysztof Kozlowski]

Driver uses threaded interrupt handler so there is no real need for
using spinlocks for synchronization.  Mutexes would do fine and are
friendlier for overall system preemptivness and real-time behavior.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/crypto/s5p-sss.c | 35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
index 7ac657f46d15..1893cf5dedc0 100644
--- a/drivers/crypto/s5p-sss.c
+++ b/drivers/crypto/s5p-sss.c
@@ -21,6 +21,7 @@
 #include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/scatterlist.h>
@@ -214,7 +215,7 @@ struct s5p_aes_dev {
 	struct tasklet_struct		tasklet;
 	struct crypto_queue		queue;
 	bool				busy;
-	spinlock_t			lock;
+	struct mutex			lock;
 };
 
 static struct s5p_aes_dev *s5p_dev;
@@ -443,11 +444,10 @@ static irqreturn_t s5p_aes_interrupt(int irq, void *dev_id)
 	int err_dma_tx = 0;
 	int err_dma_rx = 0;
 	bool tx_end = false;
-	unsigned long flags;
 	uint32_t status;
 	int err;
 
-	spin_lock_irqsave(&dev->lock, flags);
+	mutex_lock(&dev->lock);
 
 	/*
 	 * Handle rx or tx interrupt. If there is still data (scatterlist did not
@@ -481,7 +481,7 @@ static irqreturn_t s5p_aes_interrupt(int irq, void *dev_id)
 	if (tx_end) {
 		s5p_sg_done(dev);
 
-		spin_unlock_irqrestore(&dev->lock, flags);
+		mutex_unlock(&dev->lock);
 
 		s5p_aes_complete(dev, 0);
 		/* Device is still busy */
@@ -498,7 +498,7 @@ static irqreturn_t s5p_aes_interrupt(int irq, void *dev_id)
 		if (err_dma_rx == 1)
 			s5p_set_dma_indata(dev, dev->sg_src);
 
-		spin_unlock_irqrestore(&dev->lock, flags);
+		mutex_unlock(&dev->lock);
 	}
 
 	return IRQ_HANDLED;
@@ -506,7 +506,7 @@ static irqreturn_t s5p_aes_interrupt(int irq, void *dev_id)
 error:
 	s5p_sg_done(dev);
 	dev->busy = false;
-	spin_unlock_irqrestore(&dev->lock, flags);
+	mutex_unlock(&dev->lock);
 	s5p_aes_complete(dev, err);
 
 	return IRQ_HANDLED;
@@ -599,7 +599,6 @@ static void s5p_aes_crypt_start(struct s5p_aes_dev *dev, unsigned long mode)
 {
 	struct ablkcipher_request *req = dev->req;
 	uint32_t aes_control;
-	unsigned long flags;
 	int err;
 
 	aes_control = SSS_AES_KEY_CHANGE_MODE;
@@ -625,7 +624,7 @@ static void s5p_aes_crypt_start(struct s5p_aes_dev *dev, unsigned long mode)
 		    |  SSS_AES_BYTESWAP_KEY
 		    |  SSS_AES_BYTESWAP_CNT;
 
-	spin_lock_irqsave(&dev->lock, flags);
+	mutex_lock(&dev->lock);
 
 	SSS_WRITE(dev, FCINTENCLR,
 		  SSS_FCINTENCLR_BTDMAINTENCLR | SSS_FCINTENCLR_BRDMAINTENCLR);
@@ -648,7 +647,7 @@ static void s5p_aes_crypt_start(struct s5p_aes_dev *dev, unsigned long mode)
 	SSS_WRITE(dev, FCINTENSET,
 		  SSS_FCINTENSET_BTDMAINTENSET | SSS_FCINTENSET_BRDMAINTENSET);
 
-	spin_unlock_irqrestore(&dev->lock, flags);
+	mutex_unlock(&dev->lock);
 
 	return;
 
@@ -658,7 +657,7 @@ static void s5p_aes_crypt_start(struct s5p_aes_dev *dev, unsigned long mode)
 indata_error:
 	s5p_sg_done(dev);
 	dev->busy = false;
-	spin_unlock_irqrestore(&dev->lock, flags);
+	mutex_unlock(&dev->lock);
 	s5p_aes_complete(dev, err);
 }
 
@@ -667,18 +666,17 @@ static void s5p_tasklet_cb(unsigned long data)
 	struct s5p_aes_dev *dev = (struct s5p_aes_dev *)data;
 	struct crypto_async_request *async_req, *backlog;
 	struct s5p_aes_reqctx *reqctx;
-	unsigned long flags;
 
-	spin_lock_irqsave(&dev->lock, flags);
+	mutex_lock(&dev->lock);
 	backlog   = crypto_get_backlog(&dev->queue);
 	async_req = crypto_dequeue_request(&dev->queue);
 
 	if (!async_req) {
 		dev->busy = false;
-		spin_unlock_irqrestore(&dev->lock, flags);
+		mutex_unlock(&dev->lock);
 		return;
 	}
-	spin_unlock_irqrestore(&dev->lock, flags);
+	mutex_unlock(&dev->lock);
 
 	if (backlog)
 		backlog->complete(backlog, -EINPROGRESS);
@@ -693,18 +691,17 @@ static void s5p_tasklet_cb(unsigned long data)
 static int s5p_aes_handle_req(struct s5p_aes_dev *dev,
 			      struct ablkcipher_request *req)
 {
-	unsigned long flags;
 	int err;
 
-	spin_lock_irqsave(&dev->lock, flags);
+	mutex_lock(&dev->lock);
 	err = ablkcipher_enqueue_request(&dev->queue, req);
 	if (dev->busy) {
-		spin_unlock_irqrestore(&dev->lock, flags);
+		mutex_unlock(&dev->lock);
 		goto exit;
 	}
 	dev->busy = true;
 
-	spin_unlock_irqrestore(&dev->lock, flags);
+	mutex_unlock(&dev->lock);
 
 	tasklet_schedule(&dev->tasklet);
 
@@ -856,7 +853,7 @@ static int s5p_aes_probe(struct platform_device *pdev)
 		return err;
 	}
 
-	spin_lock_init(&pdata->lock);
+	mutex_init(&pdata->lock);
 
 	pdata->aes_ioaddr = pdata->ioaddr + variant->aes_offset;
 
-- 
2.9.3

Re: [PATCH 4/4] crypto: s5p-sss - Use mutex instead of spinlock

[Bartlomiej Zolnierkiewicz]

Hi,

On Friday, March 17, 2017 04:49:22 PM Krzysztof Kozlowski wrote:
> Driver uses threaded interrupt handler so there is no real need for
> using spinlocks for synchronization.  Mutexes would do fine and are
> friendlier for overall system preemptivness and real-time behavior.

Are you sure that this conversion is safe?  This driver also uses
a tasklet and tasklets run in the interrupt context.

> @@ -667,18 +666,17 @@ static void s5p_tasklet_cb(unsigned long data)
>  	struct s5p_aes_dev *dev = (struct s5p_aes_dev *)data;
>  	struct crypto_async_request *async_req, *backlog;
>  	struct s5p_aes_reqctx *reqctx;
> -	unsigned long flags;
>  
> -	spin_lock_irqsave(&dev->lock, flags);
> +	mutex_lock(&dev->lock);
>  	backlog   = crypto_get_backlog(&dev->queue);
>  	async_req = crypto_dequeue_request(&dev->queue);
>  
>  	if (!async_req) {
>  		dev->busy = false;
> -		spin_unlock_irqrestore(&dev->lock, flags);
> +		mutex_unlock(&dev->lock);
>  		return;
>  	}
> -	spin_unlock_irqrestore(&dev->lock, flags);
> +	mutex_unlock(&dev->lock);

Best regards,

Re: [PATCH 4/4] crypto: s5p-sss - Use mutex instead of spinlock

[Bartlomiej Zolnierkiewicz]

Hi,

On Friday, March 17, 2017 04:49:22 PM Krzysztof Kozlowski wrote:
> Driver uses threaded interrupt handler so there is no real need for
> using spinlocks for synchronization.  Mutexes would do fine and are
> friendlier for overall system preemptivness and real-time behavior.

Are you sure that this conversion is safe?  This driver also uses
a tasklet and tasklets run in the interrupt context.

> @@ -667,18 +666,17 @@ static void s5p_tasklet_cb(unsigned long data)
>  	struct s5p_aes_dev *dev = (struct s5p_aes_dev *)data;
>  	struct crypto_async_request *async_req, *backlog;
>  	struct s5p_aes_reqctx *reqctx;
> -	unsigned long flags;
>  
> -	spin_lock_irqsave(&dev->lock, flags);
> +	mutex_lock(&dev->lock);
>  	backlog   = crypto_get_backlog(&dev->queue);
>  	async_req = crypto_dequeue_request(&dev->queue);
>  
>  	if (!async_req) {
>  		dev->busy = false;
> -		spin_unlock_irqrestore(&dev->lock, flags);
> +		mutex_unlock(&dev->lock);
>  		return;
>  	}
> -	spin_unlock_irqrestore(&dev->lock, flags);
> +	mutex_unlock(&dev->lock);

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH 4/4] crypto: s5p-sss - Use mutex instead of spinlock

[Krzysztof Kozlowski]

On Fri, Mar 17, 2017 at 06:28:29PM +0100, Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> On Friday, March 17, 2017 04:49:22 PM Krzysztof Kozlowski wrote:
> > Driver uses threaded interrupt handler so there is no real need for
> > using spinlocks for synchronization.  Mutexes would do fine and are
> > friendlier for overall system preemptivness and real-time behavior.
> 
> Are you sure that this conversion is safe?  This driver also uses
> a tasklet and tasklets run in the interrupt context.
>

Yes, you're right. This is not safe and patch should be dropped. Thanks
for spotting this.

Best regards,
Krzysztof

Re: [PATCH 0/4] crypto: s5p-sss - Fix and minor improvements

[Herbert Xu]

On Fri, Mar 17, 2017 at 04:49:18PM +0200, Krzysztof Kozlowski wrote:
> Hi,
> 
> I still did not fix the NULL pointer dereference reported by
> Nathan Royce [1], but I got some other improvements.
> 
> Testing done on Odroid U3 (Exynos4412) with tcrypt and cryptsetup.
> 
> Best regards,
> Krzysztof

Patches 1-3 applied.  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