Patchwork Buglet in net/pkt_cls.h pointer handling.

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()'.

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.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html