diff mbox series

SAUCE: crypto: thunderx_zip: Fix fallout from CONFIG_VMAP_STACK

Message ID 1522146713-25175-2-git-send-email-paolo.pisati@canonical.com
State New
Headers show
Series SAUCE: crypto: thunderx_zip: Fix fallout from CONFIG_VMAP_STACK | expand

Commit Message

Paolo Pisati March 27, 2018, 10:31 a.m. UTC 
From: Jan Glauber <jglauber@cavium.com>

BugLink: http://bugs.launchpad.net/bugs/1755073

Enabling virtual mapped kernel stacks breaks the thunderx_zip
driver. On compression or decompression the executing CPU hangs
in an endless loop. The reason for this is the usage of __pa()
by the driver that does not work for an address that is
not part of the 1:1 mapping.

The zip driver allocates a result struct on the stack and needs
to tell the hardware the pysical address within this struct
that is used to signal the completion of the request.
With CONFIG_VMAP_STACK __pa() is no longer usable for a stack address.

As the hardware gets the wrong address it writes the result byte
to an arbitrary address. The zip driver then waits forever for the
completion byte to contain a non-zero value.

Allocating the result struct from 1:1 mapped memory resolves this
bug.

Note that there are more outstanding issues with the used completion
mechanism:
- volatile is used for a bitfield
- no barriers at all re used
- the completion loop should have a retry counter and not run forever
- polling for the result should use a delay
- interrupts for completion and error notification should be used

Signed-off-by: Jan Glauber <jglauber@cavium.com>
Signed-off-by: Paolo Pisati <paolo.pisati@canonical.com>
---
 drivers/crypto/cavium/zip/zip_crypto.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

Comments

Thadeu Lima de Souza Cascardo March 27, 2018, 8:06 p.m. UTC | #1 
Applied to bionic master-next branch.

Thanks.
Cascardo.

Acked-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>

Applied-to: bionic/master-next
Stefan Bader March 28, 2018, 9:20 a.m. UTC | #2 
On 27.03.2018 12:31, Paolo Pisati wrote:
> From: Jan Glauber <jglauber@cavium.com>
> 
> BugLink: http://bugs.launchpad.net/bugs/1755073
> 
> Enabling virtual mapped kernel stacks breaks the thunderx_zip
> driver. On compression or decompression the executing CPU hangs
> in an endless loop. The reason for this is the usage of __pa()
> by the driver that does not work for an address that is
> not part of the 1:1 mapping.
> 
> The zip driver allocates a result struct on the stack and needs
> to tell the hardware the pysical address within this struct
> that is used to signal the completion of the request.
> With CONFIG_VMAP_STACK __pa() is no longer usable for a stack address.
> 
> As the hardware gets the wrong address it writes the result byte
> to an arbitrary address. The zip driver then waits forever for the
> completion byte to contain a non-zero value.
> 
> Allocating the result struct from 1:1 mapped memory resolves this
> bug.
> 
> Note that there are more outstanding issues with the used completion
> mechanism:
> - volatile is used for a bitfield
> - no barriers at all re used
> - the completion loop should have a retry counter and not run forever
> - polling for the result should use a delay
> - interrupts for completion and error notification should be used
> 
> Signed-off-by: Jan Glauber <jglauber@cavium.com>
> Signed-off-by: Paolo Pisati <paolo.pisati@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>

> ---
>  drivers/crypto/cavium/zip/zip_crypto.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/crypto/cavium/zip/zip_crypto.c b/drivers/crypto/cavium/zip/zip_crypto.c
> index 8df4d26..2fc9b03 100644
> --- a/drivers/crypto/cavium/zip/zip_crypto.c
> +++ b/drivers/crypto/cavium/zip/zip_crypto.c
> @@ -124,7 +124,7 @@ int zip_compress(const u8 *src, unsigned int slen,
>  		 struct zip_kernel_ctx *zip_ctx)
>  {
>  	struct zip_operation  *zip_ops   = NULL;
> -	struct zip_state      zip_state;
> +	struct zip_state      *zip_state;
>  	struct zip_device     *zip = NULL;
>  	int ret;
>  
> @@ -135,20 +135,23 @@ int zip_compress(const u8 *src, unsigned int slen,
>  	if (!zip)
>  		return -ENODEV;
>  
> -	memset(&zip_state, 0, sizeof(struct zip_state));
> +	zip_state = kzalloc(sizeof(*zip_state), GFP_KERNEL);
> +	if (!zip_state)
> +		return -ENOMEM;
> +
>  	zip_ops = &zip_ctx->zip_comp;
>  
>  	zip_ops->input_len  = slen;
>  	zip_ops->output_len = *dlen;
>  	memcpy(zip_ops->input, src, slen);
>  
> -	ret = zip_deflate(zip_ops, &zip_state, zip);
> +	ret = zip_deflate(zip_ops, zip_state, zip);
>  
>  	if (!ret) {
>  		*dlen = zip_ops->output_len;
>  		memcpy(dst, zip_ops->output, *dlen);
>  	}
> -
> +	kfree(zip_state);
>  	return ret;
>  }
>  
> @@ -157,7 +160,7 @@ int zip_decompress(const u8 *src, unsigned int slen,
>  		   struct zip_kernel_ctx *zip_ctx)
>  {
>  	struct zip_operation  *zip_ops   = NULL;
> -	struct zip_state      zip_state;
> +	struct zip_state      *zip_state;
>  	struct zip_device     *zip = NULL;
>  	int ret;
>  
> @@ -168,7 +171,10 @@ int zip_decompress(const u8 *src, unsigned int slen,
>  	if (!zip)
>  		return -ENODEV;
>  
> -	memset(&zip_state, 0, sizeof(struct zip_state));
> +	zip_state = kzalloc(sizeof(*zip_state), GFP_KERNEL);
> +	if (!zip_state)
> +		return -ENOMEM;
> +
>  	zip_ops = &zip_ctx->zip_decomp;
>  	memcpy(zip_ops->input, src, slen);
>  
> @@ -179,13 +185,13 @@ int zip_decompress(const u8 *src, unsigned int slen,
>  	zip_ops->input_len  = slen;
>  	zip_ops->output_len = *dlen;
>  
> -	ret = zip_inflate(zip_ops, &zip_state, zip);
> +	ret = zip_inflate(zip_ops, zip_state, zip);
>  
>  	if (!ret) {
>  		*dlen = zip_ops->output_len;
>  		memcpy(dst, zip_ops->output, *dlen);
>  	}
> -
> +	kfree(zip_state);
>  	return ret;
>  }
>  
>
Kleber Sacilotto de Souza March 29, 2018, 2:57 p.m. UTC | #3 
On 03/27/18 12:31, Paolo Pisati wrote:
> From: Jan Glauber <jglauber@cavium.com>
> 
> BugLink: http://bugs.launchpad.net/bugs/1755073
> 
> Enabling virtual mapped kernel stacks breaks the thunderx_zip
> driver. On compression or decompression the executing CPU hangs
> in an endless loop. The reason for this is the usage of __pa()
> by the driver that does not work for an address that is
> not part of the 1:1 mapping.
> 
> The zip driver allocates a result struct on the stack and needs
> to tell the hardware the pysical address within this struct
> that is used to signal the completion of the request.
> With CONFIG_VMAP_STACK __pa() is no longer usable for a stack address.
> 
> As the hardware gets the wrong address it writes the result byte
> to an arbitrary address. The zip driver then waits forever for the
> completion byte to contain a non-zero value.
> 
> Allocating the result struct from 1:1 mapped memory resolves this
> bug.
> 
> Note that there are more outstanding issues with the used completion
> mechanism:
> - volatile is used for a bitfield
> - no barriers at all re used
> - the completion loop should have a retry counter and not run forever
> - polling for the result should use a delay
> - interrupts for completion and error notification should be used
> 
> Signed-off-by: Jan Glauber <jglauber@cavium.com>
> Signed-off-by: Paolo Pisati <paolo.pisati@canonical.com>
> ---
>  drivers/crypto/cavium/zip/zip_crypto.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/crypto/cavium/zip/zip_crypto.c b/drivers/crypto/cavium/zip/zip_crypto.c
> index 8df4d26..2fc9b03 100644
> --- a/drivers/crypto/cavium/zip/zip_crypto.c
> +++ b/drivers/crypto/cavium/zip/zip_crypto.c
> @@ -124,7 +124,7 @@ int zip_compress(const u8 *src, unsigned int slen,
>  		 struct zip_kernel_ctx *zip_ctx)
>  {
>  	struct zip_operation  *zip_ops   = NULL;
> -	struct zip_state      zip_state;
> +	struct zip_state      *zip_state;
>  	struct zip_device     *zip = NULL;
>  	int ret;
>  
> @@ -135,20 +135,23 @@ int zip_compress(const u8 *src, unsigned int slen,
>  	if (!zip)
>  		return -ENODEV;
>  
> -	memset(&zip_state, 0, sizeof(struct zip_state));
> +	zip_state = kzalloc(sizeof(*zip_state), GFP_KERNEL);
> +	if (!zip_state)
> +		return -ENOMEM;
> +
>  	zip_ops = &zip_ctx->zip_comp;
>  
>  	zip_ops->input_len  = slen;
>  	zip_ops->output_len = *dlen;
>  	memcpy(zip_ops->input, src, slen);
>  
> -	ret = zip_deflate(zip_ops, &zip_state, zip);
> +	ret = zip_deflate(zip_ops, zip_state, zip);
>  
>  	if (!ret) {
>  		*dlen = zip_ops->output_len;
>  		memcpy(dst, zip_ops->output, *dlen);
>  	}
> -
> +	kfree(zip_state);
>  	return ret;
>  }
>  
> @@ -157,7 +160,7 @@ int zip_decompress(const u8 *src, unsigned int slen,
>  		   struct zip_kernel_ctx *zip_ctx)
>  {
>  	struct zip_operation  *zip_ops   = NULL;
> -	struct zip_state      zip_state;
> +	struct zip_state      *zip_state;
>  	struct zip_device     *zip = NULL;
>  	int ret;
>  
> @@ -168,7 +171,10 @@ int zip_decompress(const u8 *src, unsigned int slen,
>  	if (!zip)
>  		return -ENODEV;
>  
> -	memset(&zip_state, 0, sizeof(struct zip_state));
> +	zip_state = kzalloc(sizeof(*zip_state), GFP_KERNEL);
> +	if (!zip_state)
> +		return -ENOMEM;
> +
>  	zip_ops = &zip_ctx->zip_decomp;
>  	memcpy(zip_ops->input, src, slen);
>  
> @@ -179,13 +185,13 @@ int zip_decompress(const u8 *src, unsigned int slen,
>  	zip_ops->input_len  = slen;
>  	zip_ops->output_len = *dlen;
>  
> -	ret = zip_inflate(zip_ops, &zip_state, zip);
> +	ret = zip_inflate(zip_ops, zip_state, zip);
>  
>  	if (!ret) {
>  		*dlen = zip_ops->output_len;
>  		memcpy(dst, zip_ops->output, *dlen);
>  	}
> -
> +	kfree(zip_state);
>  	return ret;
>  }
>  
> 

Applied to artful/master-next branch.

Thanks,
Kleber
diff mbox series

Patch

diff --git a/drivers/crypto/cavium/zip/zip_crypto.c b/drivers/crypto/cavium/zip/zip_crypto.c
index 8df4d26..2fc9b03 100644
--- a/drivers/crypto/cavium/zip/zip_crypto.c
+++ b/drivers/crypto/cavium/zip/zip_crypto.c
@@ -124,7 +124,7 @@  int zip_compress(const u8 *src, unsigned int slen,
 		 struct zip_kernel_ctx *zip_ctx)
 {
 	struct zip_operation  *zip_ops   = NULL;
-	struct zip_state      zip_state;
+	struct zip_state      *zip_state;
 	struct zip_device     *zip = NULL;
 	int ret;
 
@@ -135,20 +135,23 @@  int zip_compress(const u8 *src, unsigned int slen,
 	if (!zip)
 		return -ENODEV;
 
-	memset(&zip_state, 0, sizeof(struct zip_state));
+	zip_state = kzalloc(sizeof(*zip_state), GFP_KERNEL);
+	if (!zip_state)
+		return -ENOMEM;
+
 	zip_ops = &zip_ctx->zip_comp;
 
 	zip_ops->input_len  = slen;
 	zip_ops->output_len = *dlen;
 	memcpy(zip_ops->input, src, slen);
 
-	ret = zip_deflate(zip_ops, &zip_state, zip);
+	ret = zip_deflate(zip_ops, zip_state, zip);
 
 	if (!ret) {
 		*dlen = zip_ops->output_len;
 		memcpy(dst, zip_ops->output, *dlen);
 	}
-
+	kfree(zip_state);
 	return ret;
 }
 
@@ -157,7 +160,7 @@  int zip_decompress(const u8 *src, unsigned int slen,
 		   struct zip_kernel_ctx *zip_ctx)
 {
 	struct zip_operation  *zip_ops   = NULL;
-	struct zip_state      zip_state;
+	struct zip_state      *zip_state;
 	struct zip_device     *zip = NULL;
 	int ret;
 
@@ -168,7 +171,10 @@  int zip_decompress(const u8 *src, unsigned int slen,
 	if (!zip)
 		return -ENODEV;
 
-	memset(&zip_state, 0, sizeof(struct zip_state));
+	zip_state = kzalloc(sizeof(*zip_state), GFP_KERNEL);
+	if (!zip_state)
+		return -ENOMEM;
+
 	zip_ops = &zip_ctx->zip_decomp;
 	memcpy(zip_ops->input, src, slen);
 
@@ -179,13 +185,13 @@  int zip_decompress(const u8 *src, unsigned int slen,
 	zip_ops->input_len  = slen;
 	zip_ops->output_len = *dlen;
 
-	ret = zip_inflate(zip_ops, &zip_state, zip);
+	ret = zip_inflate(zip_ops, zip_state, zip);
 
 	if (!ret) {
 		*dlen = zip_ops->output_len;
 		memcpy(dst, zip_ops->output, *dlen);
 	}
-
+	kfree(zip_state);
 	return ret;
 }