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

[Jarek Poplawski <jarkao2@gmail.com>]

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