TKIP encryption should allocate enough tailroom

TKIP encryption should allocate enough tailroom

[Brandon Craig Rhodes]

[-- Attachment #1: Type: text/plain, Size: 1284 bytes --]

After frustrating days of hung TCP connections, I have determined that
the encryption routines in net/iee80211/ieee80211_crypt_tkip.c should
be more aggressive in providing themselves with enough packet tailroom
to perform their encryption.

They presently will only perform encryption if the packet handed to
them happens to be stored with enough tailroom already; otherwise,
they drop the packet.  If ieee80211_michael_mic_add() does not find
the eight bytes of tailroom it needs, it produces a message like

 kernel: Invalid packet for Michael MIC add (tailroom=6 hdr_len=24 skb->len=92)

and drops the packet.  The ieee80211_tkip_encrypt() function that
follows is less needy, requiring only four bytes of tailroom - but
fails to log anything at all when it drops a packet!

The attached patch, if applied to kernel 2.6.18, solves both problems.
I am not very familiar with the conventions of kernel networking code,
so there may be better ways of fixing this; but the patch should
illustrate the general idea, and perhaps provides others in my unhappy
situation with stable WPA connections until someone can provide a more
authorized patch.

And thanks for all the great code, guys; this is the first problem I
have had with the networking stack in eleven years of using Linux.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: ieee80211-tkip-tailroom.patch --]
[-- Type: text/x-diff, Size: 1376 bytes --]

--- net/ieee80211/ieee80211_crypt_tkip.c.orig	2007-01-10 10:20:40.000000000 -0500
+++ net/ieee80211/ieee80211_crypt_tkip.c	2007-01-16 21:21:51.000000000 -0500
@@ -334,9 +334,19 @@
 		return -1;
 	}
 
-	if (skb_tailroom(skb) < 4 || skb->len < hdr_len)
+	if (skb->len < hdr_len)
 		return -1;
 
+	if (skb_tailroom(skb) < 4) {
+		int err;
+		err = skb_padto(skb, skb->len + 4);
+		if (unlikely(err || skb_tailroom(skb) < 4)) {
+			printk(KERN_DEBUG "Failed to increase tailroom"
+			       " for TKIP encrypt");
+			return err || -1;
+		}
+	}
+
 	len = skb->len - hdr_len;
 	pos = skb->data + hdr_len;
 
@@ -541,13 +551,24 @@
 	struct ieee80211_tkip_data *tkey = priv;
 	u8 *pos;
 
-	if (skb_tailroom(skb) < 8 || skb->len < hdr_len) {
+	if (skb->len < hdr_len) {
 		printk(KERN_DEBUG "Invalid packet for Michael MIC add "
 		       "(tailroom=%d hdr_len=%d skb->len=%d)\n",
 		       skb_tailroom(skb), hdr_len, skb->len);
 		return -1;
 	}
 
+	/* 8 bytes needed here, and 4 for ieee80211_tkip_encrypt() */
+	if (skb_tailroom(skb) < 12) {
+		int err;
+		err = skb_padto(skb, skb->len + 12);
+		if (unlikely(err || skb_tailroom(skb) < 12)) {
+			printk(KERN_DEBUG "Failed to increase tailroom"
+			       " for Michael MIC add");
+			return err || -1;
+		}
+	}
+
 	michael_mic_hdr(skb, tkey->tx_hdr);
 	pos = skb_put(skb, 8);
 	if (michael_mic(tkey, &tkey->key[16], tkey->tx_hdr,

[-- Attachment #3: Type: text/plain, Size: 83 bytes --]


-- 
Brandon Craig Rhodes   brandon@rhodesmill.org   http://rhodesmill.org/brandon

Re: TKIP encryption should allocate enough tailroom

[Brandon Craig Rhodes]

Mitchell Blank Jr <mitch@sfgoth.com> writes:

> Brandon Craig Rhodes wrote:
>
>> +		if (unlikely(err || skb_tailroom(skb) < 4)) {
>> +			printk(KERN_DEBUG "Failed to increase tailroom"
>> +			       " for TKIP encrypt");
>> +			return err || -1;
>
> The "||" operator in C doesn't act the same way it does in perl and ruby.
> You're always returning 1 here.

Egads!  You are correct.

My intention was to preserve the value of "err" if an unsuccessful
value was returned by skb_padto(), and otherwise to return "-1" which
seemed the popular value used for errors elsewhere in the code.

 - Would the expression "err ? err : -1" have served me better?
 - Do error codes mean anything here, or should I simply say "return -1"?

-- 
Brandon Craig Rhodes   brandon@rhodesmill.org   http://rhodesmill.org/brandon

Re: TKIP encryption should allocate enough tailroom

[Mitchell Blank Jr]

Brandon Craig Rhodes wrote:
> +	if (skb_tailroom(skb) < 4) {
> +		int err;
> +		err = skb_padto(skb, skb->len + 4);
> +		if (unlikely(err || skb_tailroom(skb) < 4)) {
> +			printk(KERN_DEBUG "Failed to increase tailroom"
> +			       " for TKIP encrypt");
> +			return err || -1;

The "||" operator in C doesn't act the same way it does in perl and ruby.
You're always returning 1 here.

+			return err || -1;

...and here as well.

-Mitch

Re: TKIP encryption should allocate enough tailroom

[Michael Wu]

[-- Attachment #1: Type: text/plain, Size: 707 bytes --]

On Tuesday 16 January 2007 21:31, Brandon Craig Rhodes wrote:
> The attached patch, if applied to kernel 2.6.18, solves both problems.
> I am not very familiar with the conventions of kernel networking code,
> so there may be better ways of fixing this; but the patch should
> illustrate the general idea, and perhaps provides others in my unhappy
> situation with stable WPA connections until someone can provide a more
> authorized patch.
>
Please read <http://linux.yyz.us/patch-format.html>. You should CC 
linville@tuxdriver.com as he is the maintainer of 802.11 things and can push 
your patch upstream. Also, you should have a '\n' at the end of the new 
printks.

Thanks,
-Michael Wu

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: TKIP encryption should allocate enough tailroom

[Herbert Xu]

Brandon Craig Rhodes <brandon@rhodesmill.org> wrote:
> 
> Egads!  You are correct.
> 
> My intention was to preserve the value of "err" if an unsuccessful
> value was returned by skb_padto(), and otherwise to return "-1" which
> seemed the popular value used for errors elsewhere in the code.
> 
> - Would the expression "err ? err : -1" have served me better?
> - Do error codes mean anything here, or should I simply say "return -1"?

err ?: -1

should work.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: TKIP encryption should allocate enough tailroom

[Herbert Xu]

Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
>> - Would the expression "err ? err : -1" have served me better?
>> - Do error codes mean anything here, or should I simply say "return -1"?
> 
> err ?: -1
> 
> should work.

Actually, you should probably use a more meaningful error code
than -1.
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: TKIP encryption should allocate enough tailroom

[Brandon Craig Rhodes]

I wrote:

> After frustrating days of hung TCP connections, I have determined that
> the encryption routines in net/iee80211/ieee80211_crypt_tkip.c should
> be more aggressive in providing themselves with enough packet tailroom
> to perform their encryption.

Having further reviewed my code, I have changed my mind; the
ieee80211_crypt_tkip routines are not designed to be responsible for
creating enough headroom and tailroom.  The "hostap" driver should be
doing this.  In fact, I now see that the "hostap" driver actually
attempts to create enough headroom and tailroom, but computes them
incorrectly.

I will, per the MAINTAINERS file, head back to the hostap mailing list
with a patch for hostap_80211_tx.c - this time, CC'ing the official
maintainer, as I ought; thank you, Michael Wu, for making me aware of
that bit of courtesy.

-- 
Brandon Craig Rhodes   brandon@rhodesmill.org   http://rhodesmill.org/brandon

Re: TKIP encryption should allocate enough tailroom

[Larry Finger]

Brandon Craig Rhodes wrote:
> 
> Having further reviewed my code, I have changed my mind; the
> ieee80211_crypt_tkip routines are not designed to be responsible for
> creating enough headroom and tailroom.  The "hostap" driver should be
> doing this.  In fact, I now see that the "hostap" driver actually
> attempts to create enough headroom and tailroom, but computes them
> incorrectly.
> 
> I will, per the MAINTAINERS file, head back to the hostap mailing list
> with a patch for hostap_80211_tx.c - this time, CC'ing the official
> maintainer, as I ought; thank you, Michael Wu, for making me aware of
> that bit of courtesy.

This makes sense. I have used the bcm43xx driver with WPA-PSK TKIP encryption for at least a year,
and I have never seen this error.

Larry

Re: TKIP encryption should allocate enough tailroom

[Brandon Craig Rhodes]

Larry Finger <larry.finger@lwfinger.net> writes:

> Brandon Craig Rhodes wrote:
>
>> Having further reviewed my code, I have changed my mind; the
>> ieee80211_crypt_tkip routines are not designed to be responsible for
>> creating enough headroom and tailroom.  The "hostap" driver should be
>> doing this.
>
> This makes sense. I have used the bcm43xx driver with WPA-PSK TKIP
> encryption for at least a year, and I have never seen this error.

>From what I can tell, even people using the "hostap" driver never see
this error!  The kernel must allocate such large skb's in normal
operation that there is always more than enough tailroom for twelve
bytes of TKIP encryption.

In my case, packets are arriving on the physical machine from a Xen
virtual machine that the physical machine is hosting.  The Xen code
receiving the packets must allocate much tighter skb's than does the
normal kernel code that receives packets from hardware.  (Perhaps
because Xen knows the packet size ahead of time, whereas hardware
drivers do not?)  Searching for the error message:

 kernel: Invalid packet for Michael MIC add (tailroom=6 hdr_len=24 skb->len=92)

on Google returned, if I recall, only references to the kernel source
code itself - which always gives me the sinking feeling that I've
gotten myself into a configuration shared with no one else on earth!

-- 
Brandon Craig Rhodes   brandon@rhodesmill.org   http://rhodesmill.org/brandon

Re: TKIP encryption should allocate enough tailroom

[Pekka Pietikainen]

On Wed, Jan 17, 2007 at 11:46:35AM -0500, Brandon Craig Rhodes wrote:
> Having further reviewed my code, I have changed my mind; the
> ieee80211_crypt_tkip routines are not designed to be responsible for
> creating enough headroom and tailroom.  The "hostap" driver should be
> doing this.  In fact, I now see that the "hostap" driver actually
> attempts to create enough headroom and tailroom, but computes them
> incorrectly.
Even then, if ieee80211_tkip_encrypt() didn't produce debug output for the
not enough space-case, should that be added to catch other
potentially broken drivers?

Re: TKIP encryption should allocate enough tailroom

[Brandon Craig Rhodes]

Pekka Pietikainen <pp@ee.oulu.fi> writes:

> On Wed, Jan 17, 2007 at 11:46:35AM -0500, Brandon Craig Rhodes wrote:
>> Having further reviewed my code, I have changed my mind; the
>> ieee80211_crypt_tkip routines are not designed to be responsible for
>> creating enough headroom and tailroom.
>
> Even then, if ieee80211_tkip_encrypt() didn't produce debug output
> for the not enough space-case, should that be added to catch other
> potentially broken drivers?

I think your idea is an excellent one, and would have prevented my
having to add a half-dozen printk()'s to the code myself to discover
what was going on!

I would be happy to submit such a patch myself, but am not sure what
the local kernel conventions are regarding error messages - and the
ieee80211_crypt_tkip.c functions seem wildly inconsistent with regard
to debugging messages!  In some circumstances, debug messages are
always produced; in several others, net_ratelimit() is called to
decided whether to print an error (but why in these cases and not
others?); and in many cases, nothing is printed at all (is this
because convention would dictate that the caller discover the error
and print something out?).

If I want to generate a patch that festoons the ieee80211 functions
with informative error messages, what are the guidelines?

-- 
Brandon Craig Rhodes   brandon@rhodesmill.org   http://rhodesmill.org/brandon

Re: TKIP encryption should allocate enough tailroom

[Pekka Pietikainen]

On Thu, Jan 18, 2007 at 08:55:37AM -0500, Brandon Craig Rhodes wrote:
> to debugging messages!  In some circumstances, debug messages are
> always produced; in several others, net_ratelimit() is called to
> decided whether to print an error (but why in these cases and not
> others?); and in many cases, nothing is printed at all (is this
> because convention would dictate that the caller discover the error
> and print something out?).
> 
> If I want to generate a patch that festoons the ieee80211 functions
> with informative error messages, what are the guidelines?
My understanding is:

BUG_ON() / BUG() if it's a clear "impossible" condition ("function calling
me was wrong") null pointers/buffer lengths being inconsistent. Might even be
justified in this case? 

net_ratelimit() says:
/*
 * All net warning printk()s should be guarded by this function.
 */
int net_ratelimit(void)
{
        return __printk_ratelimit(net_msg_cost, net_msg_burst);
}

Especially important if the code path can be triggered by anyone (local user
or arbitrary packet from the network). Otherwise not that big a deal if it's
buggy code elsewhere in the kernel that causes the message to be printed.
You fix the code and you stop getting thousands of lines of debug
messages/second (which is why net_ratelimit() exists).

If it's an arbitrary packet from the network, there probably should even
be a sysctl to enable/disable debug output completely. IPv4 has:

static void ip_handle_martian_source(struct net_device *dev,
                                     struct in_device *in_dev,
                                     struct sk_buff *skb,
                                     __be32 daddr,
                                     __be32 saddr)
{
        RT_CACHE_STAT_INC(in_martian_src);
#ifdef CONFIG_IP_ROUTE_VERBOSE
        if (IN_DEV_LOG_MARTIANS(in_dev) && net_ratelimit()) {
                /*
                 *      RFC1812 recommendation, if source is martian,
                 *      the only hint is MAC header.
                 */
                printk(KERN_WARNING "martian source %u.%u.%u.%u from "
                        "%u.%u.%u.%u, on dev %s\n",
                        NIPQUAD(daddr), NIPQUAD(saddr), dev->name);
...

(so there's a #ifdef _and_ a log_martians sysctl to see debug output).
In general #ifdefs should be avoided.