Changeset View
Standalone View
sys/netinet6/frag6.c
Show First 20 Lines • Show All 60 Lines • ▼ Show 20 Lines | |||||
#include <netinet/in.h> | #include <netinet/in.h> | ||||
#include <netinet/in_var.h> | #include <netinet/in_var.h> | ||||
#include <netinet/ip6.h> | #include <netinet/ip6.h> | ||||
#include <netinet6/ip6_var.h> | #include <netinet6/ip6_var.h> | ||||
#include <netinet/icmp6.h> | #include <netinet/icmp6.h> | ||||
#include <netinet/in_systm.h> /* for ECN definitions */ | #include <netinet/in_systm.h> /* for ECN definitions */ | ||||
#include <netinet/ip.h> /* for ECN definitions */ | #include <netinet/ip.h> /* for ECN definitions */ | ||||
#include <netinet/ip_var.h> | |||||
#include <netinet/tcp.h> | |||||
#include <netinet/sctp.h> | |||||
#include <netinet/udp.h> | |||||
#include <security/mac/mac_framework.h> | #include <security/mac/mac_framework.h> | ||||
/* | /* | ||||
* Reassembly headers are stored in hash buckets. | * Reassembly headers are stored in hash buckets. | ||||
*/ | */ | ||||
#define IP6REASS_NHASH_LOG2 10 | #define IP6REASS_NHASH_LOG2 10 | ||||
#define IP6REASS_NHASH (1 << IP6REASS_NHASH_LOG2) | #define IP6REASS_NHASH (1 << IP6REASS_NHASH_LOG2) | ||||
#define IP6REASS_HMASK (IP6REASS_NHASH - 1) | #define IP6REASS_HMASK (IP6REASS_NHASH - 1) | ||||
static void frag6_enq(struct ip6asfrag *, struct ip6asfrag *, | static void frag6_enq(struct ip6asfrag *, struct ip6asfrag *, | ||||
uint32_t bucket __unused); | uint32_t bucket __unused); | ||||
static void frag6_deq(struct ip6asfrag *, uint32_t bucket __unused); | static void frag6_deq(struct ip6asfrag *, uint32_t bucket __unused); | ||||
static void frag6_insque_head(struct ip6q *, struct ip6q *, | static void frag6_insque_head(struct ip6q *, struct ip6q *, | ||||
uint32_t bucket); | uint32_t bucket); | ||||
static void frag6_remque(struct ip6q *, uint32_t bucket); | static void frag6_remque(struct ip6q *, uint32_t bucket); | ||||
static void frag6_freef(struct ip6q *, uint32_t bucket); | static void frag6_freef(struct ip6q *, uint32_t bucket); | ||||
static int ip6_validhdrchain(uint8_t, uint16_t); | |||||
struct ip6qbucket { | struct ip6qbucket { | ||||
struct ip6q ip6q; | struct ip6q ip6q; | ||||
struct mtx lock; | struct mtx lock; | ||||
int count; | int count; | ||||
}; | }; | ||||
VNET_DEFINE_STATIC(volatile u_int, frag6_nfragpackets); | VNET_DEFINE_STATIC(volatile u_int, frag6_nfragpackets); | ||||
volatile u_int frag6_nfrags = 0; | volatile u_int frag6_nfrags = 0; | ||||
▲ Show 20 Lines • Show All 229 Lines • ▼ Show 20 Lines | if (ip6f->ip6f_ident == q6->ip6q_ident && | ||||
IN6_ARE_ADDR_EQUAL(&ip6->ip6_src, &q6->ip6q_src) && | IN6_ARE_ADDR_EQUAL(&ip6->ip6_src, &q6->ip6q_src) && | ||||
IN6_ARE_ADDR_EQUAL(&ip6->ip6_dst, &q6->ip6q_dst) | IN6_ARE_ADDR_EQUAL(&ip6->ip6_dst, &q6->ip6q_dst) | ||||
#ifdef MAC | #ifdef MAC | ||||
&& mac_ip6q_match(m, q6) | && mac_ip6q_match(m, q6) | ||||
#endif | #endif | ||||
) | ) | ||||
break; | break; | ||||
/* | |||||
* If first fragment (frag offset is 0) doesn't have carry an upper | |||||
bz: doesn't have doesn't have to have a have. doesn't contain a full upper maybe? | |||||
jtlUnsubmitted Not Done Inline ActionsThis still isn't fixed: "doesn't have an upper layer..." or "doesn't contain an upper layer..." jtl: This still isn't fixed: "doesn't have an upper layer..." or "doesn't contain an upper layer..." | |||||
* layer header drop it per RFC7112 | |||||
bzUnsubmitted Done Inline ActionsSentences end in a . bz: Sentences end in a . | |||||
*/ | |||||
Not Done Inline ActionsI would even reference RFC 8200 (pg 18/19): The first fragment packet is composed of: .. (3) Extension headers, if any, and the Upper-Layer header. These headers must be in the first fragment. Note: This restricts the size of the headers through the Upper-Layer header to the MTU of the path to the packet's destinations(s). I've just worked my way through frag6.c and I start to understand what I didn't like on the current approach: (1) one call to a function would be enough the "ip6_upperlayerhdr(ip6f->ip6f_nxt)" could be done there. So I guess I am asking to collapse the initial check and the additional full search into one KPI (making the other two functions possible helper functions, though only if that makes sense code/loop wise). I haven't looked yet. Then lastly I'll probably go back and revise where/when the check is done but let's leave that for the moment as frag6.c is likely to change in the meantime. bz: I would even reference RFC 8200 (pg 18/19):
The first fragment packet is composed of:
.. | |||||
fragoff = ntohs(ip6f->ip6f_offlg & IP6F_OFF_MASK); | |||||
if (fragoff == 0 && | |||||
!ip6_validhdrchain(ip6f->ip6f_nxt, frgpartlen)) { | |||||
Done Inline ActionsThe indentation looks funny here; however, that could be a Phabricator display issue. Please verify the continuation line is tabbed to line up with the if + 4 spaces more. See style(9) for more complete details. jtl: The indentation looks funny here; however, that could be a Phabricator display issue. Please… | |||||
Done Inline ActionsLet me illustrate my concern with this change by way of example: Assume I have a packet with this set of headers: IP6, frag, dstopts, tcp. As far as I can tell, this change will not verify that the tcp header is in the first fragment. To counteract this, I think you need to add some sort of loop to recursively check headers until it reaches a ULP header, exceeds some limit (e.g. the same loop limit we have in the main header processing code), or reaches the end of the packet. jtl: Let me illustrate my concern with this change by way of example: Assume I have a packet with… | |||||
Done Inline ActionsOnly allowing fragments in the form [frag ulp] was deliberate, we were aiming for a simpler change. We have everything now to do this correctly (searching the entire chain). I will update to allow more extension headers between the frag header and the ulp thj: Only allowing fragments in the form [frag ulp] was deliberate, we were aiming for a simpler… | |||||
/* remove any fragments that have arrived in this chain */ | |||||
bzUnsubmitted Done Inline ActionsR..emove ..... ... chain. bz: R..emove ..... ... chain. | |||||
jtlUnsubmitted Not Done Inline ActionsThis comment still needs to be a full sentence. jtl: This comment still needs to be a full sentence. | |||||
if (q6 != head) | |||||
Done Inline ActionsComments need to be a full sentence. jtl: Comments need to be a full sentence. | |||||
frag6_freef(q6, hash); | |||||
goto dropfrag; | |||||
} | |||||
if (q6 == head) { | if (q6 == head) { | ||||
/* | /* | ||||
* the first fragment to arrive, create a reassembly queue. | * the first fragment to arrive, create a reassembly queue. | ||||
*/ | */ | ||||
first_frag = 1; | first_frag = 1; | ||||
/* | /* | ||||
* Enforce upper bound on number of fragmented packets | * Enforce upper bound on number of fragmented packets | ||||
Show All 38 Lines | #endif | ||||
q6->ip6q_nfrag = 0; | q6->ip6q_nfrag = 0; | ||||
} | } | ||||
/* | /* | ||||
* If it's the 1st fragment, record the length of the | * If it's the 1st fragment, record the length of the | ||||
* unfragmentable part and the next header of the fragment header. | * unfragmentable part and the next header of the fragment header. | ||||
*/ | */ | ||||
fragoff = ntohs(ip6f->ip6f_offlg & IP6F_OFF_MASK); | |||||
if (fragoff == 0) { | if (fragoff == 0) { | ||||
q6->ip6q_unfrglen = offset - sizeof(struct ip6_hdr) - | q6->ip6q_unfrglen = offset - sizeof(struct ip6_hdr) - | ||||
sizeof(struct ip6_frag); | sizeof(struct ip6_frag); | ||||
q6->ip6q_nxt = ip6f->ip6f_nxt; | q6->ip6q_nxt = ip6f->ip6f_nxt; | ||||
} | } | ||||
/* | /* | ||||
* Check that the reassembled packet would not exceed 65535 bytes | * Check that the reassembled packet would not exceed 65535 bytes | ||||
▲ Show 20 Lines • Show All 357 Lines • ▼ Show 20 Lines | if (af6->ip6af_off == 0) { | ||||
/* adjust pointer */ | /* adjust pointer */ | ||||
ip6 = mtod(m, struct ip6_hdr *); | ip6 = mtod(m, struct ip6_hdr *); | ||||
/* restore source and destination addresses */ | /* restore source and destination addresses */ | ||||
ip6->ip6_src = q6->ip6q_src; | ip6->ip6_src = q6->ip6q_src; | ||||
ip6->ip6_dst = q6->ip6q_dst; | ip6->ip6_dst = q6->ip6q_dst; | ||||
icmp6_error(m, ICMP6_TIME_EXCEEDED, | icmp6_error(m, ICMP6_TIME_EXCEEDED, | ||||
Done Inline ActionsThis isn't the right statistic for this error. We might need to teach frag6_freef to deal with different error codes. jtl: This isn't the right statistic for this error. We might need to teach frag6_freef to deal with… | |||||
ICMP6_TIME_EXCEED_REASSEMBLY, 0); | ICMP6_TIME_EXCEED_REASSEMBLY, 0); | ||||
} else | } else | ||||
m_freem(m); | m_freem(m); | ||||
free(af6, M_FTABLE); | free(af6, M_FTABLE); | ||||
} | } | ||||
frag6_remque(q6, bucket); | frag6_remque(q6, bucket); | ||||
atomic_subtract_int(&frag6_nfrags, q6->ip6q_nfrag); | atomic_subtract_int(&frag6_nfrags, q6->ip6q_nfrag); | ||||
#ifdef MAC | #ifdef MAC | ||||
▲ Show 20 Lines • Show All 185 Lines • ▼ Show 20 Lines | if (m->m_len >= offset + sizeof(struct ip6_frag)) { | ||||
if ((t = m_split(m, offset, wait)) == NULL) | if ((t = m_split(m, offset, wait)) == NULL) | ||||
return (ENOMEM); | return (ENOMEM); | ||||
m_adj(t, sizeof(struct ip6_frag)); | m_adj(t, sizeof(struct ip6_frag)); | ||||
m_cat(m, t); | m_cat(m, t); | ||||
} | } | ||||
m->m_flags |= M_FRAGMENTED; | m->m_flags |= M_FRAGMENTED; | ||||
return (0); | return (0); | ||||
} | |||||
/* | |||||
* Ensure that proto is an upper layer header and there is enough space for the | |||||
* upper layer header. | |||||
*/ | |||||
Not Done Inline ActionsThis code really should not live in frag6.c. There's a few more places in the source tree which can re-use this. Having done one of the first loops in the tree in ipfw2 after v6 support was merged and people having added more protocols to it since, please make sure these two are in sync, compatible, if not even able to share code. bz: This code really should not live in frag6.c. There's a few more places in the source tree… | |||||
static int | |||||
ip6_validhdrchain(uint8_t proto, uint16_t length) | |||||
{ | |||||
bzUnsubmitted Not Done Inline ActionsEmpty line. bz: Empty line. | |||||
switch (proto) { | |||||
Not Done Inline ActionsWhy are you initialising this to 0? tuexen: Why are you initialising this to 0? | |||||
case IPPROTO_ICMPV6: | |||||
if (length < sizeof(struct icmp6_hdr)) | |||||
return 0; | |||||
return 1; | |||||
case IPPROTO_IPV6: | |||||
tuexenUnsubmitted Done Inline ActionsI'm not sure here. Why is it enough to stop here? Don't you need to look into the IPv6 payload? tuexen: I'm not sure here. Why is it enough to stop here? Don't you need to look into the IPv6 payload? | |||||
tuexenUnsubmitted Done Inline ActionsLooked it up in RFC 7112. It is fine. tuexen: Looked it up in [[ https://tools.ietf.org/html/rfc7112#section-3 | RFC 7112 ]]. It is fine. | |||||
aeUnsubmitted Done Inline ActionsI think IPPROTO_IPV4 and IPPROTO_GRE also can be added here. ae: I think IPPROTO_IPV4 and IPPROTO_GRE also can be added here. | |||||
if (length < sizeof(struct ip6_hdr)) | |||||
return 0; | |||||
return 1; | |||||
case IPPROTO_ESP: | |||||
Not Done Inline ActionsDoesn't this require the complete fragment to be in contiguous memory? I'm not sure this is always possible. I think it would be better to require only the minimum amount of memory to access the fields to be contiguous, which would be protocol specific. tuexen: Doesn't this require the complete fragment to be in contiguous memory? I'm not sure this is… | |||||
if (length < sizeof(uint32_t)) | |||||
return 0; | |||||
return 1; | |||||
case IPPROTO_SCTP: | |||||
if (length < sizeof(struct sctphdr)) | |||||
return 0; | |||||
return 1; | |||||
case IPPROTO_TCP: | |||||
if (length < TCP_MAXHLEN) | |||||
tuexenUnsubmitted Done Inline ActionsTCP_MAXHLEN is the maximum TCP header length (60), which is not the correct check here. I guess you want to check against sizeof(struct tcphdr). tuexen: TCP_MAXHLEN is the maximum TCP header length (60), which is not the correct check here. I guess… | |||||
thjAuthorUnsubmitted Done Inline ActionsMy intention is to require the maximum TCP header length be carried in the fragment. That is how I read the definition: sys/netinet/tcp.h:150 #define TCP_MAXHLEN (0xf<<2) /* max length of header in bytes */ /* max space left for options */ thj: My intention is to require the maximum TCP header length be carried in the fragment. That is… | |||||
tuexenUnsubmitted Done Inline ActionsI see. Then put a comment there, that you require more bytes to be there than you normally have a in a TCP header. Most of the times, you above code requires even user data to be there. tuexen: I see. Then put a comment there, that you require more bytes to be there than you normally have… | |||||
aeUnsubmitted Done Inline ActionsEventually, you can get the TCP header length from a packet and check against it. ae: Eventually, you can get the TCP header length from a packet and check against it. | |||||
tuexenUnsubmitted Done Inline ActionsI agree. However, this requires changing the signature of the function. The result does not depend only on the protocol being used but also on the actual header. tuexen: I agree. However, this requires changing the signature of the function. The result does not… | |||||
return 0; | |||||
return 1; | |||||
case IPPROTO_UDP: | |||||
case IPPROTO_UDPLITE: | |||||
if (length < sizeof(struct udphdr)) | |||||
return 0; | |||||
return 1; | |||||
Not Done Inline ActionsDon't you need mtod() here? Are are sure you have at least 13 consecutive bytes in the mbuf? tuexen: Don't you need mtod() here? Are are sure you have at least 13 consecutive bytes in the mbuf? | |||||
Done Inline ActionsYes, I realised this was incorrect as soon I left to catch the bus. Should be fixed in the later rev thj: Yes, I realised this was incorrect as soon I left to catch the bus. Should be fixed in the… | |||||
default: | |||||
/* not a known upper layer protocol */ | |||||
return 0; | |||||
} | |||||
return 0; | |||||
Done Inline ActionsIn a failure, this results in a double-free. Even though this resets m, it doesn't update the caller's concept of m. To do that you need to change the prototype to something like: Then, you can do this early in the code: struct mbuf *m; m = *mp; Then, after this failure, you would do: Alternatively, you can change the semantics of the return value such that a certain return value always means the mbuf is freed. jtl: In a failure, this results in a double-free.
Even though this resets m, it doesn't update the… | |||||
} | } | ||||
Not Done Inline ActionsDon't you need mtod() here? Are are sure you have at least 1 byte in the mbuf? tuexen: Don't you need mtod() here? Are are sure you have at least 1 byte in the mbuf? | |||||
Not Done Inline ActionsI guess you want to use the corresponding field of ip on the right side. Without initialising hdrlen to 0, your compiler might have catched this... tuexen: I guess you want to use the corresponding field of `ip` on the right side. Without initialising… | |||||
Not Done Inline ActionsWhen I was putting in the comment, the code looked different. One question remains: Are you sure you have enough consecutive bytes in the mbuf? tuexen: When I was putting in the comment, the code looked different. One question remains: Are you… | |||||
Done Inline ActionsAgain, doublefree. See above. jtl: Again, doublefree. See above. | |||||
Done Inline ActionsDoublefree. See above. jtl: Doublefree. See above. | |||||
Not Done Inline ActionsI can see a possibility that this will cause strange problems in two cases:
jtl: I can see a possibility that this will cause strange problems in two cases:
1. Down the road… | |||||
Done Inline ActionsThis is unnecessary. I think you should either have a default clause in the switch statement or this, but not both. jtl: This is unnecessary. I think you should either have a `default` clause in the switch statement… |
doesn't have doesn't have to have a have. doesn't contain a full upper maybe?