Re: [PATCH] orinoco: Use shash instead of ahash for MIC calculations

[Kalle Valo]

Andy Lutomirski  writes:

> On Tue, Dec 13, 2016 at 3:35 AM, Kalle Valo  wrote:
>> Andy Lutomirski  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: sta...@vger.kernel.org # 4.9 only
>>> Reported-by: Eric Biggers 
>>> Signed-off-by: Andy Lutomirski 
>>
>> "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
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] orinoco: Use shash instead of ahash for MIC calculations

[Andy Lutomirski]

On Tue, Dec 13, 2016 at 3:35 AM, Kalle Valo  wrote:
> Andy Lutomirski  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: sta...@vger.kernel.org # 4.9 only
>> Reported-by: Eric Biggers 
>> Signed-off-by: Andy Lutomirski 
>
> "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
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] orinoco: Use shash instead of ahash for MIC calculations

[Kalle Valo]

Andy Lutomirski  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: sta...@vger.kernel.org # 4.9 only
> Reported-by: Eric Biggers 
> Signed-off-by: Andy Lutomirski 

"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
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] orinoco: Use shash instead of ahash for MIC calculations

[Eric Biggers]

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([0], hdr, sizeof(hdr));
> - sg_set_buf([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
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] orinoco: Use shash instead of ahash for MIC calculations

[Andy Lutomirski]

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: sta...@vger.kernel.org # 4.9 only
Reported-by: Eric Biggers 
Signed-off-by: Andy Lutomirski 
---

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([0], hdr, sizeof(hdr));
-   sg_set_buf([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 
+#include 
 
 #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