| Message ID | 8818c45b9ec6a04d85fabf9bb437cf119fd23659.1481575835.git.luto@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 1fef293b8a9850cfa124a53c1d8878d355010403 |
| Delegated to: | Kalle Valo |
| Headers | show |
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 <luto@kernel.org> 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: stable@vger.kernel.org # 4.9 only > Reported-by: Eric Biggers <ebiggers3@gmail.com> > Signed-off-by: Andy Lutomirski <luto@kernel.org> "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.
On Tue, Dec 13, 2016 at 3:35 AM, Kalle Valo <kvalo@codeaurora.org> wrote: > Andy Lutomirski <luto@kernel.org> 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: stable@vger.kernel.org # 4.9 only >> Reported-by: Eric Biggers <ebiggers3@gmail.com> >> Signed-off-by: Andy Lutomirski <luto@kernel.org> > > "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 <luto@amacapital.net> writes: > On Tue, Dec 13, 2016 at 3:35 AM, Kalle Valo <kvalo@codeaurora.org> wrote: >> Andy Lutomirski <luto@kernel.org> 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: stable@vger.kernel.org # 4.9 only >>> Reported-by: Eric Biggers <ebiggers3@gmail.com> >>> Signed-off-by: Andy Lutomirski <luto@kernel.org> >> >> "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.
Andrew Lutomirski <luto@kernel.org> 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: stable@vger.kernel.org # 4.9 only > Reported-by: Eric Biggers <ebiggers3@gmail.com> > Signed-off-by: Andy Lutomirski <luto@kernel.org> 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
Kalle Valo <kvalo@codeaurora.org> writes: > Andrew Lutomirski <luto@kernel.org> 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: stable@vger.kernel.org # 4.9 only >> Reported-by: Eric Biggers <ebiggers3@gmail.com> >> Signed-off-by: Andy Lutomirski <luto@kernel.org> > > 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 <kvalo@codeaurora.org> writes: > Kalle Valo <kvalo@codeaurora.org> writes: > >> Andrew Lutomirski <luto@kernel.org> 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: stable@vger.kernel.org # 4.9 only >>> Reported-by: Eric Biggers <ebiggers3@gmail.com> >>> Signed-off-by: Andy Lutomirski <luto@kernel.org> >> >> 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 <luto@kernel.org> 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: stable@vger.kernel.org # 4.9 only Reported-by: Eric Biggers <ebiggers3@gmail.com> Signed-off-by: Andy Lutomirski <luto@kernel.org> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
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;
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: stable@vger.kernel.org # 4.9 only Reported-by: Eric Biggers <ebiggers3@gmail.com> Signed-off-by: Andy Lutomirski <luto@kernel.org> --- 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(-)