Bug 247129 - [PATCH] Fix some compiler warnings in netinet alias module
Summary: [PATCH] Fix some compiler warnings in netinet alias module
Status: Open
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: freebsd-bugs (Nobody)
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2020-06-09 22:47 UTC by nikethmurali
Modified: 2022-10-17 12:34 UTC (History)
1 user (show)

See Also:


Attachments
Patch for /sys/netinet/libalias/alias.c (2.91 KB, patch)
2020-06-09 22:47 UTC, nikethmurali
no flags Details | Diff
Alternate patch for sys/netinet/libalias/alias.c (9.45 KB, patch)
2020-06-10 23:10 UTC, nikethmurali
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description nikethmurali 2020-06-09 22:47:06 UTC
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
Comment 1 Mark Johnston freebsd_committer freebsd_triage 2020-06-10 15:51:04 UTC
- 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 2 nikethmurali 2020-06-10 22:52:57 UTC
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);
Comment 3 nikethmurali 2020-06-10 23:10:58 UTC
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.
Comment 4 Mark Johnston freebsd_committer freebsd_triage 2020-06-11 17:54:20 UTC
(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.
Comment 5 Graham Perrin freebsd_committer freebsd_triage 2022-10-17 12:34:39 UTC
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>