Re: [PATCH] wusb: Stop using the stack for sg crypto scratch space

[Greg KH]

On Sun, Oct 16, 2016 at 10:17:53AM -0700, Andy Lutomirski wrote:
> On Thu, Oct 6, 2016 at 10:25 AM, Andy Lutomirski  wrote:
> > Pointing an sg list at the stack is verboten and, with
> > CONFIG_VMAP_STACK=y, will malfunction.  Use kmalloc for the wusb
> > crypto stack space instead.
> >
> > Untested -- I'm not entirely convinced that this hardware exists in
> > the wild.
> 
> Greg, could you queue this up for 4.9?  I can't guarantee that it
> works, but I can pretty much guarantee that the driver is busted
> without it.

Yes, it's in my queue, couldn't do anything until after -rc1 came out.
I'll catch up on it this week.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] wusb: Stop using the stack for sg crypto scratch space

[Andy Lutomirski]

On Thu, Oct 6, 2016 at 10:25 AM, Andy Lutomirski  wrote:
> Pointing an sg list at the stack is verboten and, with
> CONFIG_VMAP_STACK=y, will malfunction.  Use kmalloc for the wusb
> crypto stack space instead.
>
> Untested -- I'm not entirely convinced that this hardware exists in
> the wild.

Greg, could you queue this up for 4.9?  I can't guarantee that it
works, but I can pretty much guarantee that the driver is busted
without it.

>
> Signed-off-by: Andy Lutomirski 
> ---
>
> This is needed for 4.9 for wusb to have a chance of working on default x86
> configurations.
>
>  drivers/usb/wusbcore/crypto.c | 59 
> +++
>  1 file changed, 37 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/usb/wusbcore/crypto.c b/drivers/usb/wusbcore/crypto.c
> index 33acd1599e99..2db79d64e66c 100644
> --- a/drivers/usb/wusbcore/crypto.c
> +++ b/drivers/usb/wusbcore/crypto.c
> @@ -133,6 +133,13 @@ static void bytewise_xor(void *_bo, const void *_bi1, 
> const void *_bi2,
> bo[itr] = bi1[itr] ^ bi2[itr];
>  }
>
> +/* Scratch space for MAC calculations. */
> +struct wusb_mac_scratch {
> +   struct aes_ccm_b0 b0;
> +   struct aes_ccm_b1 b1;
> +   struct aes_ccm_a ax;
> +};
> +
>  /*
>   * CC-MAC function WUSB1.0[6.5]
>   *
> @@ -197,16 +204,15 @@ static void bytewise_xor(void *_bo, const void *_bi1, 
> const void *_bi2,
>   *   what sg[4] is for. Maybe there is a smarter way to do this.
>   */
>  static int wusb_ccm_mac(struct crypto_skcipher *tfm_cbc,
> -   struct crypto_cipher *tfm_aes, void *mic,
> +   struct crypto_cipher *tfm_aes,
> +   struct wusb_mac_scratch *scratch,
> +   void *mic,
> const struct aes_ccm_nonce *n,
> const struct aes_ccm_label *a, const void *b,
> size_t blen)
>  {
> int result = 0;
> SKCIPHER_REQUEST_ON_STACK(req, tfm_cbc);
> -   struct aes_ccm_b0 b0;
> -   struct aes_ccm_b1 b1;
> -   struct aes_ccm_a ax;
> struct scatterlist sg[4], sg_dst;
> void *dst_buf;
> size_t dst_size;
> @@ -218,16 +224,17 @@ static int wusb_ccm_mac(struct crypto_skcipher *tfm_cbc,
>  * These checks should be compile time optimized out
>  * ensure @a fills b1's mac_header and following fields
>  */
> -   WARN_ON(sizeof(*a) != sizeof(b1) - sizeof(b1.la));
> -   WARN_ON(sizeof(b0) != sizeof(struct aes_ccm_block));
> -   WARN_ON(sizeof(b1) != sizeof(struct aes_ccm_block));
> -   WARN_ON(sizeof(ax) != sizeof(struct aes_ccm_block));
> +   WARN_ON(sizeof(*a) != sizeof(scratch->b1) - sizeof(scratch->b1.la));
> +   WARN_ON(sizeof(scratch->b0) != sizeof(struct aes_ccm_block));
> +   WARN_ON(sizeof(scratch->b1) != sizeof(struct aes_ccm_block));
> +   WARN_ON(sizeof(scratch->ax) != sizeof(struct aes_ccm_block));
>
> result = -ENOMEM;
> zero_padding = blen % sizeof(struct aes_ccm_block);
> if (zero_padding)
> zero_padding = sizeof(struct aes_ccm_block) - zero_padding;
> -   dst_size = blen + sizeof(b0) + sizeof(b1) + zero_padding;
> +   dst_size = blen + sizeof(scratch->b0) + sizeof(scratch->b1) +
> +   zero_padding;
> dst_buf = kzalloc(dst_size, GFP_KERNEL);
> if (dst_buf == NULL) {
> printk(KERN_ERR "E: can't alloc destination buffer\n");
> @@ -237,9 +244,9 @@ static int wusb_ccm_mac(struct crypto_skcipher *tfm_cbc,
> memset(iv, 0, sizeof(iv));
>
> /* Setup B0 */
> -   b0.flags = 0x59;/* Format B0 */
> -   b0.ccm_nonce = *n;
> -   b0.lm = cpu_to_be16(0); /* WUSB1.0[6.5] sez l(m) is 0 */
> +   scratch->b0.flags = 0x59;   /* Format B0 */
> +   scratch->b0.ccm_nonce = *n;
> +   scratch->b0.lm = cpu_to_be16(0);/* WUSB1.0[6.5] sez l(m) is 0 
> */
>
> /* Setup B1
>  *
> @@ -248,12 +255,12 @@ static int wusb_ccm_mac(struct crypto_skcipher *tfm_cbc,
>  * 14'--after clarification, it means to use A's contents
>  * for MAC Header, EO, sec reserved and padding.
>  */
> -   b1.la = cpu_to_be16(blen + 14);
> -   memcpy(_header, a, sizeof(*a));
> +   scratch->b1.la = cpu_to_be16(blen + 14);
> +   memcpy(>b1.mac_header, a, sizeof(*a));
>
> sg_init_table(sg, ARRAY_SIZE(sg));
> -   sg_set_buf([0], , sizeof(b0));
> -   sg_set_buf([1], , sizeof(b1));
> +   sg_set_buf([0], >b0, sizeof(scratch->b0));
> +   sg_set_buf([1], >b1, sizeof(scratch->b1));
> sg_set_buf([2], b, blen);
> /* 0 if well behaved :) */
> sg_set_buf([3], bzero, zero_padding);
> @@ -278,11 +285,12 @@ static int wusb_ccm_mac(struct crypto_skcipher *tfm_cbc,
>  * POS Crypto API: size is assumed to be AES's block size.
>  * Thanks for documenting it -- tip 

[PATCH] wusb: Stop using the stack for sg crypto scratch space

[Andy Lutomirski]

Pointing an sg list at the stack is verboten and, with
CONFIG_VMAP_STACK=y, will malfunction.  Use kmalloc for the wusb
crypto stack space instead.

Untested -- I'm not entirely convinced that this hardware exists in
the wild.

Signed-off-by: Andy Lutomirski 
---

This is needed for 4.9 for wusb to have a chance of working on default x86
configurations.

 drivers/usb/wusbcore/crypto.c | 59 +++
 1 file changed, 37 insertions(+), 22 deletions(-)

diff --git a/drivers/usb/wusbcore/crypto.c b/drivers/usb/wusbcore/crypto.c
index 33acd1599e99..2db79d64e66c 100644
--- a/drivers/usb/wusbcore/crypto.c
+++ b/drivers/usb/wusbcore/crypto.c
@@ -133,6 +133,13 @@ static void bytewise_xor(void *_bo, const void *_bi1, 
const void *_bi2,
bo[itr] = bi1[itr] ^ bi2[itr];
 }
 
+/* Scratch space for MAC calculations. */
+struct wusb_mac_scratch {
+   struct aes_ccm_b0 b0;
+   struct aes_ccm_b1 b1;
+   struct aes_ccm_a ax;
+};
+
 /*
  * CC-MAC function WUSB1.0[6.5]
  *
@@ -197,16 +204,15 @@ static void bytewise_xor(void *_bo, const void *_bi1, 
const void *_bi2,
  *   what sg[4] is for. Maybe there is a smarter way to do this.
  */
 static int wusb_ccm_mac(struct crypto_skcipher *tfm_cbc,
-   struct crypto_cipher *tfm_aes, void *mic,
+   struct crypto_cipher *tfm_aes,
+   struct wusb_mac_scratch *scratch,
+   void *mic,
const struct aes_ccm_nonce *n,
const struct aes_ccm_label *a, const void *b,
size_t blen)
 {
int result = 0;
SKCIPHER_REQUEST_ON_STACK(req, tfm_cbc);
-   struct aes_ccm_b0 b0;
-   struct aes_ccm_b1 b1;
-   struct aes_ccm_a ax;
struct scatterlist sg[4], sg_dst;
void *dst_buf;
size_t dst_size;
@@ -218,16 +224,17 @@ static int wusb_ccm_mac(struct crypto_skcipher *tfm_cbc,
 * These checks should be compile time optimized out
 * ensure @a fills b1's mac_header and following fields
 */
-   WARN_ON(sizeof(*a) != sizeof(b1) - sizeof(b1.la));
-   WARN_ON(sizeof(b0) != sizeof(struct aes_ccm_block));
-   WARN_ON(sizeof(b1) != sizeof(struct aes_ccm_block));
-   WARN_ON(sizeof(ax) != sizeof(struct aes_ccm_block));
+   WARN_ON(sizeof(*a) != sizeof(scratch->b1) - sizeof(scratch->b1.la));
+   WARN_ON(sizeof(scratch->b0) != sizeof(struct aes_ccm_block));
+   WARN_ON(sizeof(scratch->b1) != sizeof(struct aes_ccm_block));
+   WARN_ON(sizeof(scratch->ax) != sizeof(struct aes_ccm_block));
 
result = -ENOMEM;
zero_padding = blen % sizeof(struct aes_ccm_block);
if (zero_padding)
zero_padding = sizeof(struct aes_ccm_block) - zero_padding;
-   dst_size = blen + sizeof(b0) + sizeof(b1) + zero_padding;
+   dst_size = blen + sizeof(scratch->b0) + sizeof(scratch->b1) +
+   zero_padding;
dst_buf = kzalloc(dst_size, GFP_KERNEL);
if (dst_buf == NULL) {
printk(KERN_ERR "E: can't alloc destination buffer\n");
@@ -237,9 +244,9 @@ static int wusb_ccm_mac(struct crypto_skcipher *tfm_cbc,
memset(iv, 0, sizeof(iv));
 
/* Setup B0 */
-   b0.flags = 0x59;/* Format B0 */
-   b0.ccm_nonce = *n;
-   b0.lm = cpu_to_be16(0); /* WUSB1.0[6.5] sez l(m) is 0 */
+   scratch->b0.flags = 0x59;   /* Format B0 */
+   scratch->b0.ccm_nonce = *n;
+   scratch->b0.lm = cpu_to_be16(0);/* WUSB1.0[6.5] sez l(m) is 0 */
 
/* Setup B1
 *
@@ -248,12 +255,12 @@ static int wusb_ccm_mac(struct crypto_skcipher *tfm_cbc,
 * 14'--after clarification, it means to use A's contents
 * for MAC Header, EO, sec reserved and padding.
 */
-   b1.la = cpu_to_be16(blen + 14);
-   memcpy(_header, a, sizeof(*a));
+   scratch->b1.la = cpu_to_be16(blen + 14);
+   memcpy(>b1.mac_header, a, sizeof(*a));
 
sg_init_table(sg, ARRAY_SIZE(sg));
-   sg_set_buf([0], , sizeof(b0));
-   sg_set_buf([1], , sizeof(b1));
+   sg_set_buf([0], >b0, sizeof(scratch->b0));
+   sg_set_buf([1], >b1, sizeof(scratch->b1));
sg_set_buf([2], b, blen);
/* 0 if well behaved :) */
sg_set_buf([3], bzero, zero_padding);
@@ -278,11 +285,12 @@ static int wusb_ccm_mac(struct crypto_skcipher *tfm_cbc,
 * POS Crypto API: size is assumed to be AES's block size.
 * Thanks for documenting it -- tip taken from airo.c
 */
-   ax.flags = 0x01;/* as per WUSB 1.0 spec */
-   ax.ccm_nonce = *n;
-   ax.counter = 0;
-   crypto_cipher_encrypt_one(tfm_aes, (void *), (void *));
-   bytewise_xor(mic, , iv, 8);
+   scratch->ax.flags = 0x01;   /* as per WUSB 1.0 spec */
+   scratch->ax.ccm_nonce = *n;
+   scratch->ax.counter = 0;
+   crypto_cipher_encrypt_one(tfm_aes,