| Message ID | 5842EA9CC042B141995329508713AD672105A9B0@ILMAIL1.corp.local (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
I'm not sure I could follow your discussion... > @@ -790,14 +789,17 @@ ieee80211_crypto_cs_encrypt(struct > ieee80211_tx_data *tx, > return TX_CONTINUE; > } > > - if (unlikely(skb_headroom(skb) < cs->hdr_len && > - pskb_expand_head(skb, cs->hdr_len, 0, GFP_ATOMIC))) > + if (iv_len == 0) > + return TX_CONTINUE; How can this be correct? You have a cipher scheme, so you want to encrypt, but now you're not doing that? Perhaps you should drop the frame instead? johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> How can this be correct? You have a cipher scheme, so you want to > encrypt, but now you're not doing that? Perhaps you should drop the > frame instead? > On Tx, cipher scheme is "only" available trough sta pointer and is only used in ieee80211_crypto_cs_encrypt to get security header len. Since sta pointer is NULL for bcast messages, the proposed patch get the security header length using the key->conf.iv_len instead. If the key is installed with a cs, then key->conf.iv_len is initialized with cs->hdr_len. If the key is installed without a cs, then key->conf.iv_len is 0 (hence the early exit in ieee80211_crypto_cs_encrypt) rgds, cedric -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2015-03-10 at 16:46 +0000, Cedric Izoard wrote: > > How can this be correct? You have a cipher scheme, so you want to > > encrypt, but now you're not doing that? Perhaps you should drop the > > frame instead? > > > On Tx, cipher scheme is "only" available trough sta pointer and is only > used in ieee80211_crypto_cs_encrypt to get security header len. > Since sta pointer is NULL for bcast messages, the proposed patch get the > security header length using the key->conf.iv_len instead. > > If the key is installed with a cs, then key->conf.iv_len is initialized > with cs->hdr_len. > > If the key is installed without a cs, then key->conf.iv_len is 0 (hence > the early exit in ieee80211_crypto_cs_encrypt) So in reality, you're checking "is this a CS key"? Perhaps there's a better way to do that? johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/03/2015 17:48, Johannes Berg wrote: > On Tue, 2015-03-10 at 16:46 +0000, Cedric Izoard wrote: >>> How can this be correct? You have a cipher scheme, so you want to >>> encrypt, but now you're not doing that? Perhaps you should drop the >>> frame instead? >>> >> On Tx, cipher scheme is "only" available trough sta pointer and is only >> used in ieee80211_crypto_cs_encrypt to get security header len. >> Since sta pointer is NULL for bcast messages, the proposed patch get the >> security header length using the key->conf.iv_len instead. >> >> If the key is installed with a cs, then key->conf.iv_len is initialized >> with cs->hdr_len. >> >> If the key is installed without a cs, then key->conf.iv_len is 0 (hence >> the early exit in ieee80211_crypto_cs_encrypt) > > So in reality, you're checking "is this a CS key"? Perhaps there's a > better way to do that? > > johannes > > The other option I see, would be to add a pointer to the cipher scheme in struct ieee80211_key. So that in TX cipher_scheme would be test/accessed using key ptr. sta->cipher_scheme would still be used for RX. Do you consider this as a better option ? rgds, cedric -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2015-03-10 at 17:28 +0000, Cedric Izoard wrote: > The other option I see, would be to add a pointer to the cipher scheme > in struct ieee80211_key. So that in TX cipher_scheme would be > test/accessed using key ptr. sta->cipher_scheme would still be used for RX. > Do you consider this as a better option ? A whole pointer seems a bit pointless, maybe a key flag would be better? johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Tue, 2015-03-10 at 19:33 +0000, Johannes Berg wrote: >> On Tue, 2015-03-10 at 17:28 +0000, Cedric Izoard wrote: >> >> The other option I see, would be to add a pointer to the cipher scheme >> in struct ieee80211_key. So that in TX cipher_scheme would be >> test/accessed using key ptr. sta->cipher_scheme would still be used for RX. >> Do you consider this as a better option ? > > A whole pointer seems a bit pointless, maybe a key flag would be better? I agree with Johannes - a key flag is better. Another point is that ieee80211_crypto_cs_encrypt() function is called for non cs cases. It makes sense to call it only if the key flag is set.
diff --git a/net/mac80211/wpa.c b/net/mac80211/wpa.c index 75de6fa..e5d7bfa 100644 --- a/net/mac80211/wpa.c +++ b/net/mac80211/wpa.c @@ -780,9 +780,8 @@ ieee80211_crypto_cs_encrypt(struct ieee80211_tx_data *tx, struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data; struct ieee80211_key *key = tx->key; struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb); - const struct ieee80211_cipher_scheme *cs = key->sta->cipher_scheme; int hdrlen; - u8 *pos; + u8 *pos, iv_len = key->conf.iv_len; if (info->control.hw_key &&
Do not always dereference sta to get cipher scheme as it may be null for broadcast messages. Instead get IV length from key configuration which has been initialized with cipher scheme. Signed-off-by: Cedric Izoard <cedric.izoard@ceva-dsp.com> --- net/mac80211/wpa.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) !(info->control.hw_key->flags & IEEE80211_KEY_FLAG_PUT_IV_SPACE)) { @@ -790,14 +789,17 @@ ieee80211_crypto_cs_encrypt(struct ieee80211_tx_data *tx, return TX_CONTINUE; } - if (unlikely(skb_headroom(skb) < cs->hdr_len && - pskb_expand_head(skb, cs->hdr_len, 0, GFP_ATOMIC))) + if (iv_len == 0) + return TX_CONTINUE; + + if (unlikely(skb_headroom(skb) < iv_len && + pskb_expand_head(skb, iv_len, 0, GFP_ATOMIC))) return TX_DROP; hdrlen = ieee80211_hdrlen(hdr->frame_control); - pos = skb_push(skb, cs->hdr_len); - memmove(pos, pos + cs->hdr_len, hdrlen); + pos = skb_push(skb, iv_len); + memmove(pos, pos + iv_len, hdrlen); return TX_CONTINUE; } @@ -1217,11 +1219,9 @@ ieee80211_crypto_hw_encrypt(struct ieee80211_tx_data *tx) if (!info->control.hw_key) return TX_DROP; - if (tx->key->sta->cipher_scheme) { - res = ieee80211_crypto_cs_encrypt(tx, skb); - if (res != TX_CONTINUE) - return res; - } + res = ieee80211_crypto_cs_encrypt(tx, skb); + if (res != TX_CONTINUE) + return res; } ieee80211_tx_set_protected(tx);