Created attachment 215410 [details] Patch for /sys/netinet/libalias/alias.c Patch contains fixes for warnings found when building kernel 1. /usr/src/sys/netinet/libalias/alias.c:472:12: warning: comparison of integers of different signs: 'int' and 'unsigned int' [-Wsign-compare] 2. /usr/src/sys/netinet/libalias/alias.c:1559:35: warning: taking address of packed member 'ip_src' of class or structure 'ip' may result in an unaligned pointer value [-Waddress-of-packed-member] 3./usr/src/sys/netinet/libalias/alias.c:1433:42: error: taking address of packed member 'ip_dst' of class or structure 'ip' may result in an unaligned pointer value
- Please ensure that local variables are declared at the beginning of the function, per style(9). - FragmentIn() and ProtoAliasIn() may modify the destination address, and with your patch the modification is discarded. So I think the patch is incorrect.
Comment on attachment 215410 [details] Patch for /sys/netinet/libalias/alias.c Index: alias.c =================================================================== --- alias.c (revision 361967) +++ alias.c (working copy) @@ -443,7 +443,8 @@ IcmpAliasIn(struct libalias *la, struct ip *pip) { struct icmp *ic; - int dlen, iresult; + uint16_t dlen; + int iresult; LIBALIAS_LOCK_ASSERT(la); @@ -740,7 +741,7 @@ { struct udphdr *ud; struct alias_link *lnk; - int dlen; + uint16_t dlen; LIBALIAS_LOCK_ASSERT(la); @@ -839,7 +840,8 @@ u_short dest_port; u_short proxy_server_port; int proxy_type; - int dlen, error; + uint16_t dlen; + int error; LIBALIAS_LOCK_ASSERT(la); @@ -944,7 +946,7 @@ { struct tcphdr *tc; struct alias_link *lnk; - int dlen; + uint16_t dlen; LIBALIAS_LOCK_ASSERT(la); @@ -1069,7 +1071,8 @@ static int TcpAliasOut(struct libalias *la, struct ip *pip, int maxpacketsize, int create) { - int dlen, proxy_type, error; + uint16_t dlen; + int proxy_type, error; u_short dest_port; u_short proxy_server_port; struct in_addr dest_address; @@ -1343,9 +1346,10 @@ static int LibAliasInLocked(struct libalias *la, char *ptr, int maxpacketsize) { - struct in_addr alias_addr; + struct in_addr alias_addr,ip_dst; struct ip *pip; int iresult; + u_short ip_sum ; if (la->packetAliasMode & PKT_ALIAS_REVERSE) { la->packetAliasMode &= ~PKT_ALIAS_REVERSE; @@ -1398,14 +1402,24 @@ error = find_handler(IN, IP, la, pip, &ad); if (error == 0) iresult = PKT_ALIAS_OK; - else + else { + ip_dst = pip->ip_dst; + ip_sum = pip->ip_sum; iresult = ProtoAliasIn(la, pip->ip_src, - &pip->ip_dst, pip->ip_p, &pip->ip_sum); + &ip_dst, pip->ip_p, &ip_sum); + pip->ip_dst = ip_dst; + pip->ip_sum = ip_sum; + } } break; - default: - iresult = ProtoAliasIn(la, pip->ip_src, &pip->ip_dst, - pip->ip_p, &pip->ip_sum); + default: { + ip_dst = pip->ip_dst; + ip_sum = pip->ip_sum; + iresult = ProtoAliasIn(la, pip->ip_src, &ip_dst, + pip->ip_p, &ip_sum); + } + pip->ip_dst = ip_dst; + pip->ip_sum = ip_sum; break; } @@ -1421,8 +1435,12 @@ } } } else { - iresult = FragmentIn(la, pip->ip_src, &pip->ip_dst, pip->ip_id, - &pip->ip_sum); + ip_dst = pip->ip_dst; + ip_sum = pip->ip_sum; + iresult = FragmentIn(la, pip->ip_src, &ip_dst, pip->ip_id, + &ip_sum); + pip->ip_dst = ip_dst; + pip->ip_sum = ip_sum; } getout: @@ -1479,8 +1497,9 @@ ) { int iresult; - struct in_addr addr_save; + struct in_addr addr_save,ip_src; struct ip *pip; + u_short ip_sum; if (la->packetAliasMode & PKT_ALIAS_REVERSE) { la->packetAliasMode &= ~PKT_ALIAS_REVERSE; @@ -1555,18 +1574,33 @@ error = find_handler(OUT, IP, la, pip, &ad); if (error == 0) iresult = PKT_ALIAS_OK; - else - iresult = ProtoAliasOut(la, &pip->ip_src, - pip->ip_dst, pip->ip_p, &pip->ip_sum, create); + else { + ip_src = pip->ip_src; + ip_sum = pip->ip_sum; + iresult = ProtoAliasOut(la, &ip_src, + pip->ip_dst, pip->ip_p, &ip_sum, create); + pip->ip_src = ip_src; + pip->ip_sum = ip_sum; + } } break; - default: - iresult = ProtoAliasOut(la, &pip->ip_src, - pip->ip_dst, pip->ip_p, &pip->ip_sum, create); + default: { + ip_src = pip->ip_src; + ip_sum = pip->ip_sum; + iresult = ProtoAliasOut(la, &ip_src, + pip->ip_dst, pip->ip_p, &ip_sum, create); + pip->ip_src = ip_src; + pip->ip_sum = ip_sum; + } break; } } else { - iresult = FragmentOut(la, &pip->ip_src, &pip->ip_sum); + ip_src = pip->ip_src; + ip_sum = pip->ip_sum; + iresult = FragmentOut(la, &ip_src, &ip_sum); + pip->ip_src = ip_src; + pip->ip_sum = ip_sum; + } SetDefaultAliasAddress(la, addr_save);
Created attachment 215431 [details] Alternate patch for sys/netinet/libalias/alias.c Thanks for looking at my patch and catching what I missed. I've updated the original patch to not drop updated values after function calls. ------------ As an alternate approach to this, I've attached a patch with a slightly different approach (alias.diff.2). I thought this change would make it easier to read. Please let me know what you think.
(In reply to nikethmurali from comment #3) A couple more comments: Changing dlen to be unsigned seems dangerous. For example, we have: 450 dlen = ntohs(pip->ip_len) - (pip->ip_hl << 2); 451 if (dlen < ICMP_MINLEN) 452 return (PKT_ALIAS_IGNORED); What happens if a malicious packet defines a header length longer than ip_len? If dlen is unsigned, it will end up being a large number and will pass the subsequent check. Regarding the alignment issue, wouldn't it be simpler to modify each of ProtoAliasIn/Out and FragmentIn/Out to take a struct ip * as input, and have them update fields directly? Then those functions know that the ip address fields are not necessarily self-aligned and the compiler can handle it.
Keyword: patch or patch-ready – in lieu of summary line prefix: [patch] * bulk change for the keyword * summary lines may be edited manually (not in bulk). Keyword descriptions and search interface: <https://bugs.freebsd.org/bugzilla/describekeywords.cgi>