Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757275AbcJPRSX (ORCPT ); Sun, 16 Oct 2016 13:18:23 -0400 Received: from mail-vk0-f43.google.com ([209.85.213.43]:34006 "EHLO mail-vk0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757054AbcJPRSO (ORCPT ); Sun, 16 Oct 2016 13:18:14 -0400 MIME-Version: 1.0 In-Reply-To: <7c12b3d10f457bc97e63be6c64acd87afb50aeea.1475774653.git.luto@kernel.org> References: <7c12b3d10f457bc97e63be6c64acd87afb50aeea.1475774653.git.luto@kernel.org> From: Andy Lutomirski Date: Sun, 16 Oct 2016 10:17:53 -0700 Message-ID: Subject: Re: [PATCH] wusb: Stop using the stack for sg crypto scratch space To: Andy Lutomirski Cc: Greg KH , Herbert Xu , "linux-kernel@vger.kernel.org" , USB list Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7057 Lines: 173 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(&b1.mac_header, a, sizeof(*a)); > + scratch->b1.la = cpu_to_be16(blen + 14); > + memcpy(&scratch->b1.mac_header, a, sizeof(*a)); > > sg_init_table(sg, ARRAY_SIZE(sg)); > - sg_set_buf(&sg[0], &b0, sizeof(b0)); > - sg_set_buf(&sg[1], &b1, sizeof(b1)); > + sg_set_buf(&sg[0], &scratch->b0, sizeof(scratch->b0)); > + sg_set_buf(&sg[1], &scratch->b1, sizeof(scratch->b1)); > sg_set_buf(&sg[2], b, blen); > /* 0 if well behaved :) */ > sg_set_buf(&sg[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 *)&ax, (void *)&ax); > - bytewise_xor(mic, &ax, 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, (void *)&scratch->ax, > + (void *)&scratch->ax); > + bytewise_xor(mic, &scratch->ax, iv, 8); > result = 8; > error_cbc_crypt: > kfree(dst_buf); > @@ -305,6 +313,7 @@ ssize_t wusb_prf(void *out, size_t out_size, > struct aes_ccm_nonce n = *_n; > struct crypto_skcipher *tfm_cbc; > struct crypto_cipher *tfm_aes; > + struct wusb_mac_scratch *scratch; > u64 sfn = 0; > __le64 sfn_le; > > @@ -331,17 +340,23 @@ ssize_t wusb_prf(void *out, size_t out_size, > printk(KERN_ERR "E: can't set AES key: %d\n", (int)result); > goto error_setkey_aes; > } > + scratch = kmalloc(sizeof(*scratch), GFP_KERNEL); > + if (!scratch) > + goto error_alloc_scratch; > > for (bitr = 0; bitr < (len + 63) / 64; bitr++) { > sfn_le = cpu_to_le64(sfn++); > memcpy(&n.sfn, &sfn_le, sizeof(n.sfn)); /* n.sfn++... */ > - result = wusb_ccm_mac(tfm_cbc, tfm_aes, out + bytes, > + result = wusb_ccm_mac(tfm_cbc, tfm_aes, scratch, out + bytes, > &n, a, b, blen); > if (result < 0) > goto error_ccm_mac; > bytes += result; > } > result = bytes; > + > + kfree(scratch); > +error_alloc_scratch: > error_ccm_mac: > error_setkey_aes: > crypto_free_cipher(tfm_aes); > -- > 2.7.4 > -- Andy Lutomirski AMA Capital Management, LLC