crypto: s5p-sss - Fix and minor improvements

PATCH In review Krzysztof Kozlowski 3 participants 11 messages Mar 17, 2017 – Mar 24, 2017

Subsystems (3)

Stable branch Crypto API Samsung S5P Security SubSystem (sss) driver

lore

Share & tools

lore

Krzysztof Kozlowski

author maintainer

2017-03-17 14:49 UTC #0 lore raw

cover crypto: s5p-sss - Fix and minor improvements

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/[email protected]/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(-)

Signature · 1 line

2.9.3

Krzysztof Kozlowski

author maintainer

2017-03-17 14:49 UTC #1 lore raw

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

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 <[email protected]> # v4.10.x

Fixes 28b62b145868 ("crypto: s5p-sss - Fix spinlock recursion on LRW(AES)")

Signed-off-by Krzysztof Kozlowski <[email protected]>

---
 drivers/crypto/s5p-sss.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/crypto/s5p-sss.c +3 -2

index 1b9da3dc799b..6c620487e9c2 100644
@@ -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);
 }

Signature · 1 line

2.9.3

Bartlomiej Zolnierkiewicz

Reviewed-by on v1 1/4

2017-03-17 17:57 UTC #2 lore raw

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

Quoted 15 lines · 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.

Quoted 3 lines

Cc <[email protected]> # v4.10.x
Fixes 28b62b145868 ("crypto: s5p-sss - Fix spinlock recursion on LRW(AES)")
Signed-off-by Krzysztof Kozlowski <[email protected]>

Reviewed-by Bartlomiej Zolnierkiewicz <[email protected]>

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

Krzysztof Kozlowski

author maintainer

2017-03-17 15:16 UTC #3 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/crypto/s5p-sss.c | 3 ---
 1 file changed, 3 deletions(-)

drivers/crypto/s5p-sss.c +0 -3

index 6c620487e9c2..35ea84b7d775 100644
@@ -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;

Signature · 1 line

2.9.3

Bartlomiej Zolnierkiewicz

Reviewed-by on v1 2/4

2017-03-17 17:57 UTC #4 lore raw

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

Quoted 4 lines · 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 <[email protected]>

Reviewed-by Bartlomiej Zolnierkiewicz <[email protected]>

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

Krzysztof Kozlowski

author maintainer

2017-03-17 14:49 UTC #5 lore raw

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

In reply to Krzysztof Kozlowski

Add kernel-doc to s5p_aes_dev structure.

Signed-off-by Krzysztof Kozlowski <[email protected]>

---
 drivers/crypto/s5p-sss.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

drivers/crypto/s5p-sss.c +26 -1

index 35ea84b7d775..7ac657f46d15 100644
@@ -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;

Signature · 1 line

2.9.3

Bartlomiej Zolnierkiewicz

Reviewed-by on v1 3/4

2017-03-17 17:58 UTC #6 lore raw

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

Quoted 3 lines · Krzysztof Kozlowski

Add kernel-doc to s5p_aes_dev structure.
Signed-off-by Krzysztof Kozlowski <[email protected]>

Reviewed-by Bartlomiej Zolnierkiewicz <[email protected]>

Best regards,

Krzysztof Kozlowski

author maintainer

2017-03-17 14:49 UTC #7 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/crypto/s5p-sss.c | 35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

drivers/crypto/s5p-sss.c +16 ~~-19~~

index 7ac657f46d15..1893cf5dedc0 100644
@@ -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;

Signature · 1 line

2.9.3

Bartlomiej Zolnierkiewicz

2017-03-17 17:30 UTC #8 lore raw

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

Hi,

Quoted 3 lines · 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.

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

Quoted 9 lines

drivers/crypto/s5p-sss.c +1 -2

@@ -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);

drivers/crypto/s5p-sss.c static void s5p_tasklet_cb(unsigned long data) · line 679

Quoted 10 lines

drivers/crypto/s5p-sss.c +2 -2

 	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,

Krzysztof Kozlowski

author maintainer

2017-03-17 17:54 UTC #9 lore raw

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

Quoted 11 lines · Bartlomiej Zolnierkiewicz

Hi,
Quoted 3 lines · 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.
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

Herbert Xu

maintainer

2017-03-24 14:14 UTC #10 lore raw

In reply to Krzysztof Kozlowski

Quoted 9 lines · 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

Patches 1-3 applied. Thanks.

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt