[PATCH][next] crypto: chtls: don't leak information from the stack to userspace

[Colin King]

From: Colin Ian King 

The structure crypto_info contains fields that are not initialized and
only .version is set.  The copy_to_user call is hence leaking information
from the stack to userspace which must be avoided. Fix this by zero'ing
all the unused fields.

Detected by CoverityScan, CID#1467421 ("Uninitialized scalar variable")

Fixes: a08943947873 ("crypto: chtls - Register chtls with net tls")
Signed-off-by: Colin Ian King 
---
 drivers/crypto/chelsio/chtls/chtls_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/chelsio/chtls/chtls_main.c 
b/drivers/crypto/chelsio/chtls/chtls_main.c
index 007c45c38fc7..69f3756eb980 100644
--- a/drivers/crypto/chelsio/chtls/chtls_main.c
+++ b/drivers/crypto/chelsio/chtls/chtls_main.c
@@ -441,7 +441,7 @@ static int chtls_uld_rx_handler(void *handle, const __be64 
*rsp,
 static int do_chtls_getsockopt(struct sock *sk, char __user *optval,
   int __user *optlen)
 {
-   struct tls_crypto_info crypto_info;
+   struct tls_crypto_info crypto_info = { 0 };
 
crypto_info.version = TLS_1_2_VERSION;
if (copy_to_user(optval, _info, sizeof(struct tls_crypto_info)))
-- 
2.15.1

Re: [RESEND] SHASH_DESC_ON_STACK macro

[Gustavo A. R. Silva]

Hi Herbert,

On 03/27/2018 05:07 AM, Herbert Xu wrote:

On Fri, Mar 23, 2018 at 02:09:46PM -0500, Gustavo A. R. Silva wrote:


Hi Herbert,

There is an ongoing effort to remove all VLAs from the code base [1] and
while working on that I came across the following macro at
include/crypto/hash.h:154:

#define SHASH_DESC_ON_STACK(shash, ctx)   \
 char __##shash##_desc[sizeof(struct shash_desc) + \
 crypto_shash_descsize(ctx)] CRYPTO_MINALIGN_ATTR; \
 struct shash_desc *shash = (struct shash_desc *)__##shash##_desc


Currently, this macro is being used in 46 different places.

I wonder how big can tfm->descsize can get?


descsize is capped at PAGE_SIZE / 8.



Sorry for the late response. It seems I lost track of this email somehow.

OK. So based on your response I will propose a patch for this.

Thanks!
--
Gustavo

Re: [PATCH 1/2] crypto: thunderx_zip: Fix fallout from CONFIG_VMAP_STACK

[Jan Glauber]

On Wed, Mar 28, 2018 at 03:05:56PM +0200, Jan Glauber wrote:
> 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 which does no longer 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 physical address within this struct
> that is used to signal the completion of the request.
> 
> As the hardware gets the wrong address after the broken __pa
> conversion it writes 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.

Hi Herbert,

Just realized that we might sleep in this path, so GFP_KERNEL wont
work here. Same with usleep in the second patch.

I'll respin the patches.

--Jan


> Signed-off-by: Jan Glauber 
> Reviewed-by: Robert Richter 
> Cc: stable  # 4.14
> ---
>  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(_state, 0, sizeof(struct zip_state));
> +   zip_state = kzalloc(sizeof(*zip_state), GFP_KERNEL);
> +   if (!zip_state)
> +   return -ENOMEM;
> +
> zip_ops = _ctx->zip_comp;
> 
> zip_ops->input_len  = slen;
> zip_ops->output_len = *dlen;
> memcpy(zip_ops->input, src, slen);
> 
> -   ret = zip_deflate(zip_ops, _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(_state, 0, sizeof(struct zip_state));
> +   zip_state = kzalloc(sizeof(*zip_state), GFP_KERNEL);
> +   if (!zip_state)
> +   return -ENOMEM;
> +
> zip_ops = _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, _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;
>  }
> 
> --
> 2.7.4