Message ID | 20101216215627.43f34977.suckfish@ihug.co.nz |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
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
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