The driver put a constant buffer of all zeros on the stack and
pointed a scatterlist entry at it. This doesn't work with virtual
stacks. Make the buffer static to fix it.
Cc: [email protected] # 4.9 only
Reported-by: Eric Biggers <[email protected]>
Signed-off-by: Andy Lutomirski <[email protected]>
---
drivers/usb/wusbcore/crypto.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/wusbcore/crypto.c b/drivers/usb/wusbcore/crypto.c
index 79451f7ef1b7..a7e007a0cd49 100644
--- a/drivers/usb/wusbcore/crypto.c
+++ b/drivers/usb/wusbcore/crypto.c
@@ -216,7 +216,7 @@ static int wusb_ccm_mac(struct crypto_skcipher *tfm_cbc,
struct scatterlist sg[4], sg_dst;
void *dst_buf;
size_t dst_size;
- const u8 bzero[16] = { 0 };
+ static const u8 bzero[16] = { 0 };
u8 iv[crypto_skcipher_ivsize(tfm_cbc)];
size_t zero_padding;
--
2.9.3
The driver put a constant buffer of all zeros on the stack and
pointed a scatterlist entry at it in two places. This doesn't work
with virtual stacks. Use a static 16-byte buffer of zeros instead.
Cc: [email protected] # 4.9 only
Reported-by: Eric Biggers <[email protected]>
Signed-off-by: Andy Lutomirski <[email protected]>
---
security/keys/encrypted-keys/encrypted.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
index 17a06105ccb6..fab2fb864002 100644
--- a/security/keys/encrypted-keys/encrypted.c
+++ b/security/keys/encrypted-keys/encrypted.c
@@ -46,6 +46,7 @@ static const char key_format_default[] = "default";
static const char key_format_ecryptfs[] = "ecryptfs";
static unsigned int ivsize;
static int blksize;
+static const char zero_pad[16] = {0};
#define KEY_TRUSTED_PREFIX_LEN (sizeof (KEY_TRUSTED_PREFIX) - 1)
#define KEY_USER_PREFIX_LEN (sizeof (KEY_USER_PREFIX) - 1)
@@ -481,7 +482,6 @@ static int derived_key_encrypt(struct encrypted_key_payload *epayload,
unsigned int encrypted_datalen;
u8 iv[AES_BLOCK_SIZE];
unsigned int padlen;
- char pad[16];
int ret;
encrypted_datalen = roundup(epayload->decrypted_datalen, blksize);
@@ -493,11 +493,10 @@ static int derived_key_encrypt(struct encrypted_key_payload *epayload,
goto out;
dump_decrypted_data(epayload);
- memset(pad, 0, sizeof pad);
sg_init_table(sg_in, 2);
sg_set_buf(&sg_in[0], epayload->decrypted_data,
epayload->decrypted_datalen);
- sg_set_buf(&sg_in[1], pad, padlen);
+ sg_set_buf(&sg_in[1], zero_pad, padlen);
sg_init_table(sg_out, 1);
sg_set_buf(sg_out, epayload->encrypted_data, encrypted_datalen);
@@ -584,7 +583,6 @@ static int derived_key_decrypt(struct encrypted_key_payload *epayload,
struct skcipher_request *req;
unsigned int encrypted_datalen;
u8 iv[AES_BLOCK_SIZE];
- char pad[16];
int ret;
encrypted_datalen = roundup(epayload->decrypted_datalen, blksize);
@@ -594,13 +592,12 @@ static int derived_key_decrypt(struct encrypted_key_payload *epayload,
goto out;
dump_encrypted_data(epayload, encrypted_datalen);
- memset(pad, 0, sizeof pad);
sg_init_table(sg_in, 1);
sg_init_table(sg_out, 2);
sg_set_buf(sg_in, epayload->encrypted_data, encrypted_datalen);
sg_set_buf(&sg_out[0], epayload->decrypted_data,
epayload->decrypted_datalen);
- sg_set_buf(&sg_out[1], pad, sizeof pad);
+ sg_set_buf(&sg_out[1], zero_pad, sizeof zero_pad);
memcpy(iv, epayload->iv, sizeof(iv));
skcipher_request_set_crypt(req, sg_in, sg_out, encrypted_datalen, iv);
--
2.9.3
smbencrypt() points a scatterlist to the stack, which is breaks if
CONFIG_VMAP_STACK=y.
Fix it by switching to crypto_cipher_encrypt_one(). The new code
should be considerably faster as an added benefit.
This code is nearly identical to some code that Eric Biggers
suggested.
Cc: [email protected] # 4.9 only
Reported-by: Eric Biggers <[email protected]>
Signed-off-by: Andy Lutomirski <[email protected]>
---
Compile-tested only.
fs/cifs/smbencrypt.c | 40 ++++++++--------------------------------
1 file changed, 8 insertions(+), 32 deletions(-)
diff --git a/fs/cifs/smbencrypt.c b/fs/cifs/smbencrypt.c
index 699b7868108f..c12bffefa3c9 100644
--- a/fs/cifs/smbencrypt.c
+++ b/fs/cifs/smbencrypt.c
@@ -23,7 +23,7 @@
Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
*/
-#include <crypto/skcipher.h>
+#include <linux/crypto.h>
#include <linux/module.h>
#include <linux/slab.h>
#include <linux/fs.h>
@@ -69,46 +69,22 @@ str_to_key(unsigned char *str, unsigned char *key)
static int
smbhash(unsigned char *out, const unsigned char *in, unsigned char *key)
{
- int rc;
unsigned char key2[8];
- struct crypto_skcipher *tfm_des;
- struct scatterlist sgin, sgout;
- struct skcipher_request *req;
+ struct crypto_cipher *tfm_des;
str_to_key(key, key2);
- tfm_des = crypto_alloc_skcipher("ecb(des)", 0, CRYPTO_ALG_ASYNC);
+ tfm_des = crypto_alloc_cipher("des", 0, 0);
if (IS_ERR(tfm_des)) {
- rc = PTR_ERR(tfm_des);
- cifs_dbg(VFS, "could not allocate des crypto API\n");
- goto smbhash_err;
- }
-
- req = skcipher_request_alloc(tfm_des, GFP_KERNEL);
- if (!req) {
- rc = -ENOMEM;
cifs_dbg(VFS, "could not allocate des crypto API\n");
- goto smbhash_free_skcipher;
+ return PTR_ERR(tfm_des);
}
- crypto_skcipher_setkey(tfm_des, key2, 8);
-
- sg_init_one(&sgin, in, 8);
- sg_init_one(&sgout, out, 8);
+ crypto_cipher_setkey(tfm_des, key2, 8);
+ crypto_cipher_encrypt_one(tfm_des, out, in);
+ crypto_free_cipher(tfm_des);
- skcipher_request_set_callback(req, 0, NULL, NULL);
- skcipher_request_set_crypt(req, &sgin, &sgout, 8, NULL);
-
- rc = crypto_skcipher_encrypt(req);
- if (rc)
- cifs_dbg(VFS, "could not encrypt crypt key rc: %d\n", rc);
-
- skcipher_request_free(req);
-
-smbhash_free_skcipher:
- crypto_free_skcipher(tfm_des);
-smbhash_err:
- return rc;
+ return 0;
}
static int
--
2.9.3
Eric Biggers found several crypto drivers that point scatterlists at
the stack. These drivers should never load on x86, but, for future
safety, make them depend on !VMAP_STACK.
No -stable backport should be needed as no released kernel
configuration should be affected.
Reported-by: Eric Biggers <[email protected]>
Signed-off-by: Andy Lutomirski <[email protected]>
---
drivers/crypto/Kconfig | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
index 4d2b81f2b223..481e67e54ffd 100644
--- a/drivers/crypto/Kconfig
+++ b/drivers/crypto/Kconfig
@@ -245,7 +245,7 @@ config CRYPTO_DEV_TALITOS
select CRYPTO_BLKCIPHER
select CRYPTO_HASH
select HW_RANDOM
- depends on FSL_SOC
+ depends on FSL_SOC && !VMAP_STACK
help
Say 'Y' here to use the Freescale Security Engine (SEC)
to offload cryptographic algorithm computation.
@@ -357,7 +357,7 @@ config CRYPTO_DEV_PICOXCELL
config CRYPTO_DEV_SAHARA
tristate "Support for SAHARA crypto accelerator"
- depends on ARCH_MXC && OF
+ depends on ARCH_MXC && OF && !VMAP_STACK
select CRYPTO_BLKCIPHER
select CRYPTO_AES
select CRYPTO_ECB
@@ -410,7 +410,7 @@ endif # if CRYPTO_DEV_UX500
config CRYPTO_DEV_BFIN_CRC
tristate "Support for Blackfin CRC hardware"
- depends on BF60x
+ depends on BF60x && !VMAP_STACK
help
Newer Blackfin processors have CRC hardware. Select this if you
want to use the Blackfin CRC module.
@@ -487,7 +487,7 @@ source "drivers/crypto/qat/Kconfig"
config CRYPTO_DEV_QCE
tristate "Qualcomm crypto engine accelerator"
- depends on (ARCH_QCOM || COMPILE_TEST) && HAS_DMA && HAS_IOMEM
+ depends on (ARCH_QCOM || COMPILE_TEST) && HAS_DMA && HAS_IOMEM && !VMAP_STACK
select CRYPTO_AES
select CRYPTO_DES
select CRYPTO_ECB
--
2.9.3
Eric Biggers pointed out that the orinoco driver pointed scatterlists
at the stack.
Fix it by switching from ahash to shash. The result should be
simpler, faster, and more correct.
Cc: [email protected] # 4.9 only
Reported-by: Eric Biggers <[email protected]>
Signed-off-by: Andy Lutomirski <[email protected]>
---
Compile-tested only.
drivers/net/wireless/intersil/orinoco/mic.c | 44 +++++++++++++++----------
drivers/net/wireless/intersil/orinoco/mic.h | 3 +-
drivers/net/wireless/intersil/orinoco/orinoco.h | 4 +--
3 files changed, 30 insertions(+), 21 deletions(-)
diff --git a/drivers/net/wireless/intersil/orinoco/mic.c b/drivers/net/wireless/intersil/orinoco/mic.c
index bc7397d709d3..08bc7822f820 100644
--- a/drivers/net/wireless/intersil/orinoco/mic.c
+++ b/drivers/net/wireless/intersil/orinoco/mic.c
@@ -16,7 +16,7 @@
/********************************************************************/
int orinoco_mic_init(struct orinoco_private *priv)
{
- priv->tx_tfm_mic = crypto_alloc_ahash("michael_mic", 0,
+ priv->tx_tfm_mic = crypto_alloc_shash("michael_mic", 0,
CRYPTO_ALG_ASYNC);
if (IS_ERR(priv->tx_tfm_mic)) {
printk(KERN_DEBUG "orinoco_mic_init: could not allocate "
@@ -25,7 +25,7 @@ int orinoco_mic_init(struct orinoco_private *priv)
return -ENOMEM;
}
- priv->rx_tfm_mic = crypto_alloc_ahash("michael_mic", 0,
+ priv->rx_tfm_mic = crypto_alloc_shash("michael_mic", 0,
CRYPTO_ALG_ASYNC);
if (IS_ERR(priv->rx_tfm_mic)) {
printk(KERN_DEBUG "orinoco_mic_init: could not allocate "
@@ -40,17 +40,16 @@ int orinoco_mic_init(struct orinoco_private *priv)
void orinoco_mic_free(struct orinoco_private *priv)
{
if (priv->tx_tfm_mic)
- crypto_free_ahash(priv->tx_tfm_mic);
+ crypto_free_shash(priv->tx_tfm_mic);
if (priv->rx_tfm_mic)
- crypto_free_ahash(priv->rx_tfm_mic);
+ crypto_free_shash(priv->rx_tfm_mic);
}
-int orinoco_mic(struct crypto_ahash *tfm_michael, u8 *key,
+int orinoco_mic(struct crypto_shash *tfm_michael, u8 *key,
u8 *da, u8 *sa, u8 priority,
u8 *data, size_t data_len, u8 *mic)
{
- AHASH_REQUEST_ON_STACK(req, tfm_michael);
- struct scatterlist sg[2];
+ SHASH_DESC_ON_STACK(desc, tfm_michael);
u8 hdr[ETH_HLEN + 2]; /* size of header + padding */
int err;
@@ -67,18 +66,27 @@ int orinoco_mic(struct crypto_ahash *tfm_michael, u8 *key,
hdr[ETH_ALEN * 2 + 2] = 0;
hdr[ETH_ALEN * 2 + 3] = 0;
- /* Use scatter gather to MIC header and data in one go */
- sg_init_table(sg, 2);
- sg_set_buf(&sg[0], hdr, sizeof(hdr));
- sg_set_buf(&sg[1], data, data_len);
+ desc->tfm = tfm_michael;
+ desc->flags = 0;
- if (crypto_ahash_setkey(tfm_michael, key, MIC_KEYLEN))
- return -1;
+ err = crypto_shash_setkey(tfm_michael, key, MIC_KEYLEN);
+ if (err)
+ return err;
+
+ err = crypto_shash_init(desc);
+ if (err)
+ return err;
+
+ err = crypto_shash_update(desc, hdr, sizeof(hdr));
+ if (err)
+ return err;
+
+ err = crypto_shash_update(desc, data, data_len);
+ if (err)
+ return err;
+
+ err = crypto_shash_final(desc, mic);
+ shash_desc_zero(desc);
- ahash_request_set_tfm(req, tfm_michael);
- ahash_request_set_callback(req, 0, NULL, NULL);
- ahash_request_set_crypt(req, sg, mic, data_len + sizeof(hdr));
- err = crypto_ahash_digest(req);
- ahash_request_zero(req);
return err;
}
diff --git a/drivers/net/wireless/intersil/orinoco/mic.h b/drivers/net/wireless/intersil/orinoco/mic.h
index ce731d05cc98..e8724e889219 100644
--- a/drivers/net/wireless/intersil/orinoco/mic.h
+++ b/drivers/net/wireless/intersil/orinoco/mic.h
@@ -6,6 +6,7 @@
#define _ORINOCO_MIC_H_
#include <linux/types.h>
+#include <crypto/hash.h>
#define MICHAEL_MIC_LEN 8
@@ -15,7 +16,7 @@ struct crypto_ahash;
int orinoco_mic_init(struct orinoco_private *priv);
void orinoco_mic_free(struct orinoco_private *priv);
-int orinoco_mic(struct crypto_ahash *tfm_michael, u8 *key,
+int orinoco_mic(struct crypto_shash *tfm_michael, u8 *key,
u8 *da, u8 *sa, u8 priority,
u8 *data, size_t data_len, u8 *mic);
diff --git a/drivers/net/wireless/intersil/orinoco/orinoco.h b/drivers/net/wireless/intersil/orinoco/orinoco.h
index 2f0c84b1c440..5fa1c3e3713f 100644
--- a/drivers/net/wireless/intersil/orinoco/orinoco.h
+++ b/drivers/net/wireless/intersil/orinoco/orinoco.h
@@ -152,8 +152,8 @@ struct orinoco_private {
u8 *wpa_ie;
int wpa_ie_len;
- struct crypto_ahash *rx_tfm_mic;
- struct crypto_ahash *tx_tfm_mic;
+ struct crypto_shash *rx_tfm_mic;
+ struct crypto_shash *tx_tfm_mic;
unsigned int wpa_enabled:1;
unsigned int tkip_cm_active:1;
--
2.9.3
On Mon, Dec 12, 2016 at 12:52:45PM -0800, Andy Lutomirski wrote:
> The driver put a constant buffer of all zeros on the stack and
> pointed a scatterlist entry at it. This doesn't work with virtual
> stacks. Make the buffer static to fix it.
>
> Cc: [email protected] # 4.9 only
> Reported-by: Eric Biggers <[email protected]>
> Signed-off-by: Andy Lutomirski <[email protected]>
> ---
> drivers/usb/wusbcore/crypto.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/wusbcore/crypto.c b/drivers/usb/wusbcore/crypto.c
> index 79451f7ef1b7..a7e007a0cd49 100644
> --- a/drivers/usb/wusbcore/crypto.c
> +++ b/drivers/usb/wusbcore/crypto.c
> @@ -216,7 +216,7 @@ static int wusb_ccm_mac(struct crypto_skcipher *tfm_cbc,
> struct scatterlist sg[4], sg_dst;
> void *dst_buf;
> size_t dst_size;
> - const u8 bzero[16] = { 0 };
> + static const u8 bzero[16] = { 0 };
Hm, can static memory handle DMA? That's a requirement of the USB
stack, does this data later end up being sent down to a USB host
controller?
thanks,
greg k-h
Andy Lutomirski <[email protected]> wrote:
> +static const char zero_pad[16] = {0};
Isn't there a global page of zeros or something that we can share? Also, you
shouldn't explicitly initialise it so that it stays in .bss.
> - sg_set_buf(&sg_out[1], pad, sizeof pad);
> + sg_set_buf(&sg_out[1], zero_pad, sizeof zero_pad);
Can you put brackets on the sizeof?
Thanks,
David
On Mon, Dec 12, 2016 at 1:44 PM, Greg KH <[email protected]> wrote:
> On Mon, Dec 12, 2016 at 12:52:45PM -0800, Andy Lutomirski wrote:
>> The driver put a constant buffer of all zeros on the stack and
>> pointed a scatterlist entry at it. This doesn't work with virtual
>> stacks. Make the buffer static to fix it.
>>
>> Cc: [email protected] # 4.9 only
>> Reported-by: Eric Biggers <[email protected]>
>> Signed-off-by: Andy Lutomirski <[email protected]>
>> ---
>> drivers/usb/wusbcore/crypto.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/wusbcore/crypto.c b/drivers/usb/wusbcore/crypto.c
>> index 79451f7ef1b7..a7e007a0cd49 100644
>> --- a/drivers/usb/wusbcore/crypto.c
>> +++ b/drivers/usb/wusbcore/crypto.c
>> @@ -216,7 +216,7 @@ static int wusb_ccm_mac(struct crypto_skcipher *tfm_cbc,
>> struct scatterlist sg[4], sg_dst;
>> void *dst_buf;
>> size_t dst_size;
>> - const u8 bzero[16] = { 0 };
>> + static const u8 bzero[16] = { 0 };
>
> Hm, can static memory handle DMA? That's a requirement of the USB
> stack, does this data later end up being sent down to a USB host
> controller?
I think it doesn't, but I'll switch it to use empty_zero_page instead.
--Andy
On Mon, Dec 12, 2016 at 2:28 PM, David Howells <[email protected]> wrote:
> Andy Lutomirski <[email protected]> wrote:
>
>> +static const char zero_pad[16] = {0};
>
> Isn't there a global page of zeros or something that we can share? Also, you
> shouldn't explicitly initialise it so that it stays in .bss.
This is a double-edged sword. It seems that omitting the
initialization causes it to go in .bss, which isn't read only. I have
no idea why initializing make a difference at all -- the IMO sensible
behavior would be to put it in .rodata as NOBITS either way.
But I'll use empty_zero_page.
>
>> - sg_set_buf(&sg_out[1], pad, sizeof pad);
>> + sg_set_buf(&sg_out[1], zero_pad, sizeof zero_pad);
>
> Can you put brackets on the sizeof?
Will do for v2.
On Mon, Dec 12, 2016 at 12:55:20PM -0800, Andy Lutomirski wrote:
> Eric Biggers found several crypto drivers that point scatterlists at
> the stack. These drivers should never load on x86, but, for future
> safety, make them depend on !VMAP_STACK.
>
> No -stable backport should be needed as no released kernel
> configuration should be affected.
>
> Reported-by: Eric Biggers <[email protected]>
> Signed-off-by: Andy Lutomirski <[email protected]>
Nack. These drivers are all async and are never used with a
stack request.
Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Mon, Dec 12, 2016 at 12:55:55PM -0800, Andy Lutomirski wrote:
> +int orinoco_mic(struct crypto_shash *tfm_michael, u8 *key,
> u8 *da, u8 *sa, u8 priority,
> u8 *data, size_t data_len, u8 *mic)
> {
> - AHASH_REQUEST_ON_STACK(req, tfm_michael);
> - struct scatterlist sg[2];
> + SHASH_DESC_ON_STACK(desc, tfm_michael);
> u8 hdr[ETH_HLEN + 2]; /* size of header + padding */
> int err;
>
> @@ -67,18 +66,27 @@ int orinoco_mic(struct crypto_ahash *tfm_michael, u8 *key,
> hdr[ETH_ALEN * 2 + 2] = 0;
> hdr[ETH_ALEN * 2 + 3] = 0;
>
> - /* Use scatter gather to MIC header and data in one go */
> - sg_init_table(sg, 2);
> - sg_set_buf(&sg[0], hdr, sizeof(hdr));
> - sg_set_buf(&sg[1], data, data_len);
> + desc->tfm = tfm_michael;
> + desc->flags = 0;
>
> - if (crypto_ahash_setkey(tfm_michael, key, MIC_KEYLEN))
> - return -1;
> + err = crypto_shash_setkey(tfm_michael, key, MIC_KEYLEN);
> + if (err)
> + return err;
> +
> + err = crypto_shash_init(desc);
> + if (err)
> + return err;
> +
> + err = crypto_shash_update(desc, hdr, sizeof(hdr));
> + if (err)
> + return err;
> +
> + err = crypto_shash_update(desc, data, data_len);
> + if (err)
> + return err;
> +
> + err = crypto_shash_final(desc, mic);
> + shash_desc_zero(desc);
>
> - ahash_request_set_tfm(req, tfm_michael);
> - ahash_request_set_callback(req, 0, NULL, NULL);
> - ahash_request_set_crypt(req, sg, mic, data_len + sizeof(hdr));
> - err = crypto_ahash_digest(req);
> - ahash_request_zero(req);
> return err;
It's probably a good idea to always do shash_desc_zero(), even when something
above it fails. Otherwise this looks fine. Thanks for sending these patches!
Eric
Andy Lutomirski <[email protected]> writes:
> Eric Biggers pointed out that the orinoco driver pointed scatterlists
> at the stack.
>
> Fix it by switching from ahash to shash. The result should be
> simpler, faster, and more correct.
>
> Cc: [email protected] # 4.9 only
> Reported-by: Eric Biggers <[email protected]>
> Signed-off-by: Andy Lutomirski <[email protected]>
"more correct"? Does this fix a real user visible bug or what? And why
just stable 4.9, does this maybe have something to do with
CONFIG_VMAP_STACK?
I'm just wondering should I push this to 4.10 or -next. This is a driver
for ancient hardware so I'm starting to lean for -next.
--
Kalle Valo
Hello!
On 12/12/2016 11:54 PM, Andy Lutomirski wrote:
> smbencrypt() points a scatterlist to the stack, which is breaks if
s/is//.
> CONFIG_VMAP_STACK=y.
>
> Fix it by switching to crypto_cipher_encrypt_one(). The new code
> should be considerably faster as an added benefit.
>
> This code is nearly identical to some code that Eric Biggers
> suggested.
>
> Cc: [email protected] # 4.9 only
> Reported-by: Eric Biggers <[email protected]>
> Signed-off-by: Andy Lutomirski <[email protected]>
[...]
MBR, Sergei
From: Andy Lutomirski
> Sent: 12 December 2016 20:53
> The driver put a constant buffer of all zeros on the stack and
> pointed a scatterlist entry at it in two places. This doesn't work
> with virtual stacks. Use a static 16-byte buffer of zeros instead.
...
I didn't think you could dma from static data either.
David
On Mon, 2016-12-12 at 12:54 -0800, Andy Lutomirski wrote:
> smbencrypt() points a scatterlist to the stack, which is breaks if
> CONFIG_VMAP_STACK=y.
>
> Fix it by switching to crypto_cipher_encrypt_one(). The new code
> should be considerably faster as an added benefit.
>
> This code is nearly identical to some code that Eric Biggers
> suggested.
>
> Cc: [email protected] # 4.9 only
> Reported-by: Eric Biggers <[email protected]>
> Signed-off-by: Andy Lutomirski <[email protected]>
> ---
>
> Compile-tested only.
>
> fs/cifs/smbencrypt.c | 40 ++++++++--------------------------------
> 1 file changed, 8 insertions(+), 32 deletions(-)
>
> diff --git a/fs/cifs/smbencrypt.c b/fs/cifs/smbencrypt.c
> index 699b7868108f..c12bffefa3c9 100644
> --- a/fs/cifs/smbencrypt.c
> +++ b/fs/cifs/smbencrypt.c
> @@ -23,7 +23,7 @@
> Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> */
>
> -#include <crypto/skcipher.h>
> +#include <linux/crypto.h>
> #include <linux/module.h>
> #include <linux/slab.h>
> #include <linux/fs.h>
> @@ -69,46 +69,22 @@ str_to_key(unsigned char *str, unsigned char *key)
> static int
> smbhash(unsigned char *out, const unsigned char *in, unsigned char *key)
> {
> - int rc;
> unsigned char key2[8];
> - struct crypto_skcipher *tfm_des;
> - struct scatterlist sgin, sgout;
> - struct skcipher_request *req;
> + struct crypto_cipher *tfm_des;
>
> str_to_key(key, key2);
>
> - tfm_des = crypto_alloc_skcipher("ecb(des)", 0, CRYPTO_ALG_ASYNC);
> + tfm_des = crypto_alloc_cipher("des", 0, 0);
> if (IS_ERR(tfm_des)) {
> - rc = PTR_ERR(tfm_des);
> - cifs_dbg(VFS, "could not allocate des crypto API\n");
> - goto smbhash_err;
> - }
> -
> - req = skcipher_request_alloc(tfm_des, GFP_KERNEL);
> - if (!req) {
> - rc = -ENOMEM;
> cifs_dbg(VFS, "could not allocate des crypto API\n");
> - goto smbhash_free_skcipher;
> + return PTR_ERR(tfm_des);
> }
>
> - crypto_skcipher_setkey(tfm_des, key2, 8);
> -
> - sg_init_one(&sgin, in, 8);
> - sg_init_one(&sgout, out, 8);
> + crypto_cipher_setkey(tfm_des, key2, 8);
> + crypto_cipher_encrypt_one(tfm_des, out, in);
> + crypto_free_cipher(tfm_des);
>
> - skcipher_request_set_callback(req, 0, NULL, NULL);
> - skcipher_request_set_crypt(req, &sgin, &sgout, 8, NULL);
> -
> - rc = crypto_skcipher_encrypt(req);
> - if (rc)
> - cifs_dbg(VFS, "could not encrypt crypt key rc: %d\n", rc);
> -
> - skcipher_request_free(req);
> -
> -smbhash_free_skcipher:
> - crypto_free_skcipher(tfm_des);
> -smbhash_err:
> - return rc;
> + return 0;
> }
>
> static int
Acked-by: Jeff Layton <[email protected]>
[add some people who might know]
On Tue, Dec 13, 2016 at 4:20 AM, David Laight <[email protected]> wrote:
> From: Andy Lutomirski
>> Sent: 12 December 2016 20:53
>> The driver put a constant buffer of all zeros on the stack and
>> pointed a scatterlist entry at it in two places. This doesn't work
>> with virtual stacks. Use a static 16-byte buffer of zeros instead.
> ...
>
> I didn't think you could dma from static data either.
According to lib/dma-debug.c, you can't dma to or from kernel text or
rodata, but you can dma to or from kernel bss or data. So
empty_zero_page should be okay, because it's not rodata right now.
But I think this is rather silly. Joerg, Linus, etc: would it be okay
to change lib/dma-debug.c to allow DMA *from* rodata? After all,
rodata is ordinary memory, is backed by struct page, etc. And DMA
from the zero page had better be okay because I think it happens if
you mmap some zeros, don't write to them, and then direct I/O them to
a device. Then I could also move empty_zero_page to rodata.
--Andy
On Tue, Dec 13, 2016 at 3:35 AM, Kalle Valo <[email protected]> wrote:
> Andy Lutomirski <[email protected]> writes:
>
>> Eric Biggers pointed out that the orinoco driver pointed scatterlists
>> at the stack.
>>
>> Fix it by switching from ahash to shash. The result should be
>> simpler, faster, and more correct.
>>
>> Cc: [email protected] # 4.9 only
>> Reported-by: Eric Biggers <[email protected]>
>> Signed-off-by: Andy Lutomirski <[email protected]>
>
> "more correct"? Does this fix a real user visible bug or what? And why
> just stable 4.9, does this maybe have something to do with
> CONFIG_VMAP_STACK?
Whoops, I had that text in some other patches but forgot to put it in
this one. It'll blow up with CONFIG_VMAP_STACK=y if a debug option
like CONFIG_DEBUG_VIRTUAL=y is set. It may work by accident if
debugging is off.
--Andy
Andy Lutomirski <[email protected]> wrote:
> After all, rodata is ordinary memory, is backed by struct page, etc.
Is that actually true? I thought some arches excluded the kernel image from
the page struct array to make the array consume less memory.
David
Andy Lutomirski <[email protected]> writes:
> On Tue, Dec 13, 2016 at 3:35 AM, Kalle Valo <[email protected]> wrote:
>> Andy Lutomirski <[email protected]> writes:
>>
>>> Eric Biggers pointed out that the orinoco driver pointed scatterlists
>>> at the stack.
>>>
>>> Fix it by switching from ahash to shash. The result should be
>>> simpler, faster, and more correct.
>>>
>>> Cc: [email protected] # 4.9 only
>>> Reported-by: Eric Biggers <[email protected]>
>>> Signed-off-by: Andy Lutomirski <[email protected]>
>>
>> "more correct"? Does this fix a real user visible bug or what? And why
>> just stable 4.9, does this maybe have something to do with
>> CONFIG_VMAP_STACK?
>
> Whoops, I had that text in some other patches but forgot to put it in
> this one. It'll blow up with CONFIG_VMAP_STACK=y if a debug option
> like CONFIG_DEBUG_VIRTUAL=y is set. It may work by accident if
> debugging is off.
Makes sense now, thanks. I'll add that to the commit log and queue this
to 4.10.
--
Kalle Valo
On Tue, Dec 13, 2016 at 8:45 AM, David Howells <[email protected]> wrote:
> Andy Lutomirski <[email protected]> wrote:
>
>> After all, rodata is ordinary memory, is backed by struct page, etc.
>
> Is that actually true? I thought some arches excluded the kernel image from
> the page struct array to make the array consume less memory.
I don't know whether you're right, but that sounds a bit silly to me.
This is a *tiny* amount of memory.
But there's yet another snag. Alpha doesn't have empty_zero_page --
it only has ZERO_PAGE. I could do page_address(ZERO_PAGE(0))...
--Andy
Andy Lutomirski <[email protected]> wrote:
> I don't know whether you're right, but that sounds a bit silly to me.
> This is a *tiny* amount of memory.
Assuming a 1MiB kernel image in 4K pages, that gets you back a couple of pages
I think - useful if you've only got a few MiB of RAM.
David
merged into cifs-2.6.git for-next
On Tue, Dec 13, 2016 at 7:07 AM, Jeff Layton <[email protected]> wrote:
> On Mon, 2016-12-12 at 12:54 -0800, Andy Lutomirski wrote:
>> smbencrypt() points a scatterlist to the stack, which is breaks if
>> CONFIG_VMAP_STACK=y.
>>
>> Fix it by switching to crypto_cipher_encrypt_one(). The new code
>> should be considerably faster as an added benefit.
>>
>> This code is nearly identical to some code that Eric Biggers
>> suggested.
>>
>> Cc: [email protected] # 4.9 only
>> Reported-by: Eric Biggers <[email protected]>
>> Signed-off-by: Andy Lutomirski <[email protected]>
>> ---
>>
>> Compile-tested only.
>>
>> fs/cifs/smbencrypt.c | 40 ++++++++--------------------------------
>> 1 file changed, 8 insertions(+), 32 deletions(-)
>>
>> diff --git a/fs/cifs/smbencrypt.c b/fs/cifs/smbencrypt.c
>> index 699b7868108f..c12bffefa3c9 100644
>> --- a/fs/cifs/smbencrypt.c
>> +++ b/fs/cifs/smbencrypt.c
>> @@ -23,7 +23,7 @@
>> Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
>> */
>>
>> -#include <crypto/skcipher.h>
>> +#include <linux/crypto.h>
>> #include <linux/module.h>
>> #include <linux/slab.h>
>> #include <linux/fs.h>
>> @@ -69,46 +69,22 @@ str_to_key(unsigned char *str, unsigned char *key)
>> static int
>> smbhash(unsigned char *out, const unsigned char *in, unsigned char *key)
>> {
>> - int rc;
>> unsigned char key2[8];
>> - struct crypto_skcipher *tfm_des;
>> - struct scatterlist sgin, sgout;
>> - struct skcipher_request *req;
>> + struct crypto_cipher *tfm_des;
>>
>> str_to_key(key, key2);
>>
>> - tfm_des = crypto_alloc_skcipher("ecb(des)", 0, CRYPTO_ALG_ASYNC);
>> + tfm_des = crypto_alloc_cipher("des", 0, 0);
>> if (IS_ERR(tfm_des)) {
>> - rc = PTR_ERR(tfm_des);
>> - cifs_dbg(VFS, "could not allocate des crypto API\n");
>> - goto smbhash_err;
>> - }
>> -
>> - req = skcipher_request_alloc(tfm_des, GFP_KERNEL);
>> - if (!req) {
>> - rc = -ENOMEM;
>> cifs_dbg(VFS, "could not allocate des crypto API\n");
>> - goto smbhash_free_skcipher;
>> + return PTR_ERR(tfm_des);
>> }
>>
>> - crypto_skcipher_setkey(tfm_des, key2, 8);
>> -
>> - sg_init_one(&sgin, in, 8);
>> - sg_init_one(&sgout, out, 8);
>> + crypto_cipher_setkey(tfm_des, key2, 8);
>> + crypto_cipher_encrypt_one(tfm_des, out, in);
>> + crypto_free_cipher(tfm_des);
>>
>> - skcipher_request_set_callback(req, 0, NULL, NULL);
>> - skcipher_request_set_crypt(req, &sgin, &sgout, 8, NULL);
>> -
>> - rc = crypto_skcipher_encrypt(req);
>> - if (rc)
>> - cifs_dbg(VFS, "could not encrypt crypt key rc: %d\n", rc);
>> -
>> - skcipher_request_free(req);
>> -
>> -smbhash_free_skcipher:
>> - crypto_free_skcipher(tfm_des);
>> -smbhash_err:
>> - return rc;
>> + return 0;
>> }
>>
>> static int
>
> Acked-by: Jeff Layton <[email protected]>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Thanks,
Steve
On Tue, Dec 13, 2016 at 08:40:00AM -0800, Andy Lutomirski wrote:
> But I think this is rather silly. Joerg, Linus, etc: would it be okay
> to change lib/dma-debug.c to allow DMA *from* rodata?
Yeah, this would be fine for DMA_TO_DEVICE mappings. At least I can't
think of a reason right now to not allow it, in the end its also
read-only memory for the CPU, so it can be readable by devices too.
There is no danger of race conditions like on stacks or data leaks, as
there is only compile-time data in rodata.
Joerg
Andrew Lutomirski <[email protected]> wrote:
> Eric Biggers pointed out that the orinoco driver pointed scatterlists
> at the stack.
>
> Fix it by switching from ahash to shash. The result should be
> simpler, faster, and more correct.
>
> Cc: [email protected] # 4.9 only
> Reported-by: Eric Biggers <[email protected]>
> Signed-off-by: Andy Lutomirski <[email protected]>
11 patches applied to wireless-drivers-next.git, thanks.
1fef293b8a98 orinoco: Use shash instead of ahash for MIC calculations
a08b98196a36 rt2800: make rx ampdu_factor depend on number of rx chains
e49abb19d1bf rt2800: don't set ht parameters for non-aggregated frames
a51b89698ccc rt2800: set minimum MPDU and PSDU lengths to sane values
8f03a7c6e7f9 rt2800: set MAX_PSDU len according to remote STAs capabilities
8845254112ac rt2800: rename adjust_freq_offset function
bc0077053948 rt2800: warn if doing VCO recalibration for unknow RF chip
24d42ef3b152 rt2800: perform VCO recalibration for RF5592 chip
d96324703ffa rt2x00: merge agc and vco works with link tuner
eb79a8fe94c8 rt2800: replace mdelay by usleep on vco calibration.
31369c323ba0 rt2800: replace msleep() with usleep_range() on channel switch
--
https://patchwork.kernel.org/patch/9471353/
Documentation about submitting wireless patches and checking status
from patchwork:
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Kalle Valo <[email protected]> writes:
> Andrew Lutomirski <[email protected]> wrote:
>> Eric Biggers pointed out that the orinoco driver pointed scatterlists
>> at the stack.
>>
>> Fix it by switching from ahash to shash. The result should be
>> simpler, faster, and more correct.
>>
>> Cc: [email protected] # 4.9 only
>> Reported-by: Eric Biggers <[email protected]>
>> Signed-off-by: Andy Lutomirski <[email protected]>
>
> 11 patches applied to wireless-drivers-next.git, thanks.
>
> 1fef293b8a98 orinoco: Use shash instead of ahash for MIC calculations
> a08b98196a36 rt2800: make rx ampdu_factor depend on number of rx chains
> e49abb19d1bf rt2800: don't set ht parameters for non-aggregated frames
> a51b89698ccc rt2800: set minimum MPDU and PSDU lengths to sane values
> 8f03a7c6e7f9 rt2800: set MAX_PSDU len according to remote STAs capabilities
> 8845254112ac rt2800: rename adjust_freq_offset function
> bc0077053948 rt2800: warn if doing VCO recalibration for unknow RF chip
> 24d42ef3b152 rt2800: perform VCO recalibration for RF5592 chip
> d96324703ffa rt2x00: merge agc and vco works with link tuner
> eb79a8fe94c8 rt2800: replace mdelay by usleep on vco calibration.
> 31369c323ba0 rt2800: replace msleep() with usleep_range() on channel switch
Oh man, when I was applying rt2800 patches I did an off by one error
with my patchwork script ('commit 2-12' vs 'commit 1-11') and
accidentally applied this orinoco patch to wireless-drivers-next along
with the 10 rt2800 patches above. And failed to spot that before pushing
the tree :(
As this orinoco patch is pretty important I'll cherry pick it manually
to wireless-drivers also so that it goes to 4.10. This means that the
patch is in both trees, but just with a different commit id.
Sorry for the mess.
--
Kalle Valo
Kalle Valo <[email protected]> writes:
> Kalle Valo <[email protected]> writes:
>
>> Andrew Lutomirski <[email protected]> wrote:
>>> Eric Biggers pointed out that the orinoco driver pointed scatterlists
>>> at the stack.
>>>
>>> Fix it by switching from ahash to shash. The result should be
>>> simpler, faster, and more correct.
>>>
>>> Cc: [email protected] # 4.9 only
>>> Reported-by: Eric Biggers <[email protected]>
>>> Signed-off-by: Andy Lutomirski <[email protected]>
>>
>> 11 patches applied to wireless-drivers-next.git, thanks.
>>
>> 1fef293b8a98 orinoco: Use shash instead of ahash for MIC calculations
>> a08b98196a36 rt2800: make rx ampdu_factor depend on number of rx chains
>> e49abb19d1bf rt2800: don't set ht parameters for non-aggregated frames
>> a51b89698ccc rt2800: set minimum MPDU and PSDU lengths to sane values
>> 8f03a7c6e7f9 rt2800: set MAX_PSDU len according to remote STAs capabilities
>> 8845254112ac rt2800: rename adjust_freq_offset function
>> bc0077053948 rt2800: warn if doing VCO recalibration for unknow RF chip
>> 24d42ef3b152 rt2800: perform VCO recalibration for RF5592 chip
>> d96324703ffa rt2x00: merge agc and vco works with link tuner
>> eb79a8fe94c8 rt2800: replace mdelay by usleep on vco calibration.
>> 31369c323ba0 rt2800: replace msleep() with usleep_range() on channel switch
>
> Oh man, when I was applying rt2800 patches I did an off by one error
> with my patchwork script ('commit 2-12' vs 'commit 1-11') and
> accidentally applied this orinoco patch to wireless-drivers-next along
> with the 10 rt2800 patches above. And failed to spot that before pushing
> the tree :(
>
> As this orinoco patch is pretty important I'll cherry pick it manually
> to wireless-drivers also so that it goes to 4.10. This means that the
> patch is in both trees, but just with a different commit id.
This is the commit in wireless-drivers:
commit 570b90fa230b8021f51a67fab2245fe8df6fe37d
Author: Andrew Lutomirski <[email protected]>
Date: Mon Dec 12 12:55:55 2016 -0800
orinoco: Use shash instead of ahash for MIC calculations
Eric Biggers pointed out that the orinoco driver pointed
scatterlists
at the stack.
Fix it by switching from ahash to shash. The result should be
simpler, faster, and more correct.
kvalo: cherry picked from commit
1fef293b8a9850cfa124a53c1d8878d355010403 as I
accidentally applied this patch to wireless-drivers-next when I was
supposed to
apply this wireless-drivers
Cc: [email protected] # 4.9 only
Reported-by: Eric Biggers <[email protected]>
Signed-off-by: Andy Lutomirski <[email protected]>
Signed-off-by: Kalle Valo <[email protected]>
--
Kalle Valo