[PATCH v4] crypto: caam - map src buffer before access

[PATCH v4] crypto: caam - map src buffer before access

[Yashpal Dutta]

KMap the buffers before copying trailing bytes during hmac into a session
temporary buffer. This is required if pinned buffer from user-space is send
during hmac and is safe even if hmac request is generated from within kernel.

Cc:stable@vger.kernel.org
Signed-off-by: Yashpal Dutta <yashpal.dutta@freescale.com>
Reviewed-by: Vakul Garg <vakul@freescale.com>
Reviewed-by: Horia Geanta <horia.geanta@freescale.com> 
---
Patch covers review comments on first version of patch.
./scripts/checkpatch.pl --strict 0001-crypto-caam-map-src-buffer-before-access.patch 

total: 0 errors, 0 warnings, 0 checks, 62 lines checked

The patch fixes driver crashes when a pinned buffer from user-space is sent to CAAM driver
for HMAC processing.

Added Cc:stable@vger.kernel.org

 drivers/crypto/caam/sg_sw_sec4.h |   34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/drivers/crypto/caam/sg_sw_sec4.h b/drivers/crypto/caam/sg_sw_sec4.h
index e0037c8..6d21a12 100644
--- a/drivers/crypto/caam/sg_sw_sec4.h
+++ b/drivers/crypto/caam/sg_sw_sec4.h
@@ -117,6 +117,21 @@ static int dma_unmap_sg_chained(struct device *dev, struct scatterlist *sg,
 	return nents;
 }
 
+/* Map SG page in kernel virtual address space and copy */
+static inline void sg_map_copy(u8 *dest, struct scatterlist *sg,
+			       int len, int offset)
+{
+	u8 *mapped_addr;
+
+	/*
+	 * Page here can be user-space pinned using get_user_pages
+	 * Same must be kmapped before use and kunmapped subsequently
+	 */
+	mapped_addr = kmap_atomic(sg_page(sg));
+	memcpy(dest, mapped_addr + offset, len);
+	kunmap_atomic(mapped_addr);
+}
+
 /* Copy from len bytes of sg to dest, starting from beginning */
 static inline void sg_copy(u8 *dest, struct scatterlist *sg, unsigned int len)
 {
@@ -124,15 +139,15 @@ static inline void sg_copy(u8 *dest, struct scatterlist *sg, unsigned int len)
 	int cpy_index = 0, next_cpy_index = current_sg->length;
 
 	while (next_cpy_index < len) {
-		memcpy(dest + cpy_index, (u8 *) sg_virt(current_sg),
-		       current_sg->length);
+		sg_map_copy(dest + cpy_index, current_sg, current_sg->length,
+			    current_sg->offset);
 		current_sg = scatterwalk_sg_next(current_sg);
 		cpy_index = next_cpy_index;
 		next_cpy_index += current_sg->length;
 	}
 	if (cpy_index < len)
-		memcpy(dest + cpy_index, (u8 *) sg_virt(current_sg),
-		       len - cpy_index);
+		sg_map_copy(dest + cpy_index, current_sg, len-cpy_index,
+			    current_sg->offset);
 }
 
 /* Copy sg data, from to_skip to end, to dest */
@@ -140,7 +155,7 @@ static inline void sg_copy_part(u8 *dest, struct scatterlist *sg,
 				      int to_skip, unsigned int end)
 {
 	struct scatterlist *current_sg = sg;
-	int sg_index, cpy_index;
+	int sg_index, cpy_index, offset;
 
 	sg_index = current_sg->length;
 	while (sg_index <= to_skip) {
@@ -148,9 +163,10 @@ static inline void sg_copy_part(u8 *dest, struct scatterlist *sg,
 		sg_index += current_sg->length;
 	}
 	cpy_index = sg_index - to_skip;
-	memcpy(dest, (u8 *) sg_virt(current_sg) +
-	       current_sg->length - cpy_index, cpy_index);
-	current_sg = scatterwalk_sg_next(current_sg);
-	if (end - sg_index)
+	offset = current_sg->offset + current_sg->length - cpy_index;
+	sg_map_copy(dest, current_sg, cpy_index, offset);
+	if (end - sg_index) {
+		current_sg = scatterwalk_sg_next(current_sg);
 		sg_copy(dest + cpy_index, current_sg, end - sg_index);
+	}
 }
-- 
1.7.10.4

Re: [PATCH v4] crypto: caam - map src buffer before access

[Kim Phillips]

On Sat, 21 Sep 2013 14:26:35 +0530
Yashpal Dutta <yashpal.dutta@freescale.com> wrote:

> KMap the buffers before copying trailing bytes during hmac into a session
> temporary buffer. This is required if pinned buffer from user-space is send
> during hmac and is safe even if hmac request is generated from within kernel.

it may be "safe" but it adversely affects performance for AF_ALG users,
no?

why does ocf-linux need this, and not AF_ALG?  Is a patch to ocf-linux
more appropriate here?

> Cc:stable@vger.kernel.org

fyi, this violates the following rule in
Documentation/stable_kernel_rules.txt:

 - It or an equivalent fix must already exist in Linus' tree (upstream).

Kim

Re: [PATCH v4] crypto: caam - map src buffer before access

[Horia Geantă]

On 9/23/2013 9:51 PM, Kim Phillips wrote:
> On Sat, 21 Sep 2013 14:26:35 +0530
> Yashpal Dutta <yashpal.dutta@freescale.com> wrote:
>
>> KMap the buffers before copying trailing bytes during hmac into a session
>> temporary buffer. This is required if pinned buffer from user-space is send
>> during hmac and is safe even if hmac request is generated from within kernel.
> it may be "safe" but it adversely affects performance for AF_ALG users,
> no?
>
> why does ocf-linux need this, and not AF_ALG?  Is a patch to ocf-linux
> more appropriate here?

SW hashing (crypto/ahash.c, crypto/shash.c) do the kmap/kunmap.
Crypto engine drivers should do this too. Either by themselves or 
(probably better) try to use existing support in crypto/scatterwalk.c

At the interface level, AF_ALG issues get_user_pages via 
af_alg_make_sg(), similar to what ocf-linux does.

>
>> Cc:stable@vger.kernel.org
> fyi, this violates the following rule in
> Documentation/stable_kernel_rules.txt:
>
>   - It or an equivalent fix must already exist in Linus' tree (upstream).

AFAICT, rules are more flexible, at least that's my understanding. 
Adding a Cc:stable in the signed-off area is more convenient, since it 
provides for automatic inclusion in -stable tree (once patch reaches 
Linus' tree):
  - To have the patch automatically included in the stable tree, add the tag
      Cc: stable@vger.kernel.org
    in the sign-off area. Once the patch is merged it will be applied to
    the stable tree without anything else needing to be done by the author
    or subsystem maintainer.

Horia