netdev - Re: [PATCH] cls_flow: add sanity check for the packet length

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]

Date:	Wed, 4 Aug 2010 21:51:31 +0800
From:	Changli Gao <xiaosuo@...il.com>
To:	hadi@...erus.ca
Cc:	Patrick McHardy <kaber@...sh.net>,
	"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH] cls_flow: add sanity check for the packet length

On Wed, Aug 4, 2010 at 9:39 PM, jamal <hadi@...erus.ca> wrote:
> On Wed, 2010-08-04 at 15:08 +0800, Changli Gao wrote:
>> The packet length should be checked before the packet data is dereferenced.
>>
>> Signed-off-by: Changli Gao <xiaosuo@...il.com>
>> ---
>>  include/linux/skbuff.h |    6 ++--
>>  net/sched/cls_flow.c   |   69 +++++++++++++++++++++++++++++++++----------------
>>  2 files changed, 50 insertions(+), 25 deletions(-)
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index d20d9e7..3f0ac50 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -1210,12 +1210,12 @@ static inline unsigned char *pskb_pull(struct sk_buff *skb, unsigned int len)
>>       return unlikely(len > skb->len) ? NULL : __pskb_pull(skb, len);
>>  }
>>
>> -static inline int pskb_may_pull(struct sk_buff *skb, unsigned int len)
>> +static inline bool pskb_may_pull(struct sk_buff *skb, unsigned int len)
>>  {
>>       if (likely(len <= skb_headlen(skb)))
>> -             return 1;
>> +             return true;
>>       if (unlikely(len > skb->len))
>> -             return 0;
>> +             return false;
>>       return __pskb_pull_tail(skb, len - skb_headlen(skb)) != NULL;
>>  }
>
> Above is a different patch?

Patrick thinks such things don't worth individual patches. I remember
my such patch was applied by David. It is for netdev, not netfilter. I
should obey David's rules. I'll remove this in the next version.
Thanks.

>
>> diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c
>> index f73542d..d63a374 100644
>> --- a/net/sched/cls_flow.c
>> +++ b/net/sched/cls_flow.c
>> @@ -65,37 +65,48 @@ static inline u32 addr_fold(void *addr)
>>       return (a & 0xFFFFFFFF) ^ (BITS_PER_LONG > 32 ? a >> 32 : 0);
>>  }
>>
>> -static u32 flow_get_src(const struct sk_buff *skb)
>> +static inline bool flow_pskb_may_pull(struct sk_buff *skb, unsigned int len)
>> +{
>> +     return pskb_may_pull(skb, skb_network_offset(skb) + len);
>> +}
>
> network_pskb_may_pull() would make it more generic (probably in
> skbuff.h) and reused everywhere you need skb_network_offset
>

I think pskb_network_may_pull() is better.

>
>> +static u32 flow_get_src(struct sk_buff *skb)
>>  {
>>       switch (skb->protocol) {
>>       case htons(ETH_P_IP):
>> -             return ntohl(ip_hdr(skb)->saddr);
>> +             return flow_pskb_may_pull(skb, sizeof(struct iphdr)) ?
>> +                    ntohl(ip_hdr(skb)->saddr) : 0;
>>       case htons(ETH_P_IPV6):
>> -             return ntohl(ipv6_hdr(skb)->saddr.s6_addr32[3]);
>> +             return flow_pskb_may_pull(skb, sizeof(struct ipv6hdr)) ?
>> +                    ntohl(ipv6_hdr(skb)->saddr.s6_addr32[3]) : 0;
>>       default:
>
>
> BTW - does returning zero above make sense or is returning the default below better?
>
>>               return addr_fold(skb->sk);
>
> Same comment applies in all other switch statements..
>

Oh, good suggestion. Thanks.

-- 
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