netdev - Re: Buglet in net/pkt_cls.h pointer handling.

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: <20101220095440.GA7977@ff.dom.local>
Date:	Mon, 20 Dec 2010 09:54:40 +0000
From:	Jarek Poplawski <jarkao2@...il.com>
To:	Ralph Loader <suckfish@...g.co.nz>
Cc:	netdev@...r.kernel.org
Subject: Re: Buglet in net/pkt_cls.h pointer handling.

On 2010-12-16 09:56, Ralph Loader wrote:
> Hi,
> 
> tcf_valid_offset() in net/pkt_cls.h appears to have a couple of 
> problems (obvious patch below):
> 
> (a) there is no check for overflow in the pointer arithmetic.
> (b) the pointers are presumably likely to be normally valid, so the
>     hint should be 'likely()' not 'unlikely()'.
Hi,

Your 'unlikely()' concern seems likely right. Forcing 'len >= 0' in
your patch is another question. Anyway, I wonder why don't you add
your "Signed-off-by", and Cc people who know these things: the
'TC CLASSIFIER' maintainer (as in MAINTAINERS) and the ematch author?

Cheers,
Jarek P.

> 
> The offsets used to construct the arguments to that function, e.g., as
> called in net/sched/em_u32.c, I think come from user-space & in theory
> could be crafted to cause an invalid pointer deref if ptr+len overflows?
> 
> Possibly the '<' and '>' in that function should be '<=' and '>='
> also.  I'm not familiar enough with the data-structures to be sure.
> 
> Also a question:  in em_u32.c em_u32_match(), and in cls_u32.c
> u32_classify(), we dereference pointers that have had an offset
> (originally from user space) added to them.  I can't see anything that
> keeps those pointers aligned.  Is that a problem on architectures that
> don't support unaligned pointers, or am I missing something?
> 
> Cheers,
> Ralph.
> 
> 
> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> index dd3031a..99a2d7b 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
> @@ -323,7 +323,7 @@ static inline unsigned char * tcf_get_base_ptr(struct sk_buff *skb, int layer)
>  static inline int tcf_valid_offset(const struct sk_buff *skb,
>                                    const unsigned char *ptr, const int len)
>  {
> -       return unlikely((ptr + len) < skb_tail_pointer(skb) && ptr > skb->head);
> +       return likely((ptr + len) < skb_tail_pointer(skb) && ptr > skb->head && ptr <= ptr + len);
>  }
>  
>  #ifdef CONFIG_NET_CLS_IND
--
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