On Mon, Oct 17, 2016 at 12:18 PM, Sowmini Varadhan
<sowmini.varad...@oracle.com> wrote:
> On (10/17/16 11:15), Alexander Duyck wrote:
>> I would say you probably only need the first check here for skb->data
>> and could probably skip the second part. You will be testing for
>> skb_tail_pointer in all the other tests you added so this check is
>> redundant anyway.
>>
>> Also you might want to go through and wrap these with unlikely() since
>> most of these are exception cases.
>
> Ok.. v3 will have this.
>
>> > /* Currently only IPv4/IPv6 with TCP is supported */
>> > switch (hdr.ipv4->version) {
>> > case IPVERSION:
>> > /* access ihl as u8 to avoid unaligned access on ia64 */
>> > hlen = (hdr.network[0] & 0x0F) << 2;
>> > + if (skb_tail_pointer(skb) < hdr.network + hlen +
>> > + sizeof(struct tcphdr))
>> > + return;
>> > l4_proto = hdr.ipv4->protocol;
>> > break;
>> > case 6:
>> > hlen = hdr.network - skb->data;
>> > + if (skb_tail_pointer(skb) < hdr.network + hlen +
>> > + sizeof(struct tcphdr))
>> > + return;
>> > l4_proto = ipv6_find_hdr(skb, &hlen, IPPROTO_TCP, NULL,
>> > NULL);
>> > hlen -= hdr.network - skb->data;
>> > break;
>>
>> I believe one more check is needed after this block to verify the TCP
>> header fields are present.
>>
>> So you probably need to add a check for "skb_tail_pointer(skb) <
>> (hdr.network + hlen + 20)".
>
> But isnt that the same thing as the checks before l4_proto computation above?
Sort of. The problem is IPv6 can include extension headers and that
can totally mess with us. So we need to do one more check to verify
that we have enough space for IPv6 w/ TCP which would be hdr.raw + 20
+ hlenl.
Thanks.
- Alex
