netdev - Re: [PATCH] cls_u32: use skb_copy_bits() to dereference data safely

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <AANLkTilkweiJPmQOUv78lKL9ohZo1StzlJcBw0johYi0@mail.gmail.com>
Date:	Wed, 2 Jun 2010 01:47:00 +0800
From:	Changli Gao <xiaosuo@...il.com>
To:	hadi@...erus.ca
Cc:	"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH] cls_u32: use skb_copy_bits() to dereference data safely

On Tue, Jun 1, 2010 at 8:34 PM, jamal <hadi@...erus.ca> wrote:
> Hi Changli,
>
> On Mon, 2010-05-31 at 10:24 +0800, Changli Gao wrote:
>> use skb_copy_bits() to dereference data safely
>>
>> the original skb->data dereference isn't safe, as there isn't any skb->len or
>> skb_is_nonlinear() check.
>
> I dont see any safety issue in current code in this respect. Do you have
> a specific scenario where this would be unsafe? We inspect in 32 bit
> chunks walking the packet data and stop when there is no more packet
> data.

I added the following debug code into cls_u32.c

                for (i = n->sel.nkeys; i>0; i--, key++) {
+                       int off;
+
+                       off = key->off+(off2&key->offmask) + (ptr - skb->data);
+                       if (off + 4 > skb->len)
+                               printk("skb->len: %d, off: %d\n",
skb->len, off);

                        if
((*(__be32*)(ptr+key->off+(off2&key->offmask))^key->val)&key->mask) {
                                n = n->next;
@@ -179,8 +186,13 @@ check_terminal:

                ht = n->ht_down;
                sel = 0;
-               if (ht->divisor)
+               if (ht->divisor) {
+                       int off = ptr - skb->data + n->sel.hoff;
+                       if (off + 4 > skb->len)
+                               printk("skb->len: %d, off: %d\n", skb->len,
+                                       off);
                        sel =
ht->divisor&u32_hash_fold(*(__be32*)(ptr+n->sel.hoff),
&n->sel,n->fshift);
+               }

the tc filter command is:

tc filter add dev eth0 parent 8001:0 proto ip  u32 match u32
0x00000000 0x0000ffff at 40

Then I execute nc to connect to an UDP port:

localhost linux # nc -u 192.168.235.1 80

localhost linux #

just press enter.

I got these demsg:

[ 7525.330604] skb->len: 43, off: 54

we are trying to access the memory out of this skb.

>
>> skb_copy_bits() is used instead in this patch. And
>> when the skb isn't long enough, we terminate the function u32_classify()
>> immediately with -1.
>>
>
> That could be a very interesting optimization - but i see it as _very
> hard_ to do with current u32 given it has branching and different
> branches would have different lengths etc. You'd have to essentially do
> some math in user space as to what min length would suffice given
> a specified filter and pass that to the kernel.
>

It isn't an optimization, but an error exit. :)

-- 
Regards,
Changli Gao(xiaosuo@...il.com)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html