Skip to contentSkip to conversation

cipso: subtraction on unsigned hdr_delta

PATCH In review Roel Kluin 3 participants 3 messages Mar 3, 2009 – Mar 4, 2009

Subsystems (3)
Networking Network drivers Labeled networking (NetLabel)
lore
Share & tools
lore

Roel Kluin

author
#0 lore raw

PATCH 1/1 cipso: subtraction on unsigned hdr_delta

hdr_delta is unsigned, so take care not to subtract below 0.

Signed-off-by Roel Kluin <[email protected]>

---

net/ipv4/cipso_ipv4.c +5 -1
index 7bc9929..b1d862b 100644
@@ -1999,7 +1999,11 @@ void cipso_v4_sock_delattr(struct sock *sk)
 				iter++;
 		hdr_delta = opt->optlen;
 		opt->optlen = (optlen_new + 3) & ~3;
-		hdr_delta -= opt->optlen;
+
+		if (hdr_delta > opt->optlen)
+			hdr_delta -= opt->optlen;
+		else
+			hdr_delta = 0;
 	} else {
 		/* only the cipso option was present on the socket so we can
 		 * remove the entire option struct */
maintainer
#1 lore raw

From: Roel Kluin <[email protected]>
Date: Tue, 03 Mar 2009 23:23:38 +0100

Quoted 3 lines

hdr_delta is unsigned, so take care not to subtract below 0.

Signed-off-by Roel Kluin <[email protected]>

I'd like Paul to look at this.

It looks like it does the wrong thing when the option space increases, which would have made hdr_delta negative if it were of a signed type.

I think therefore that sk_conn->icsk_ext_hdr_len needs to be adjusted in both directions, just just when the option area length shrinks.

#2 lore raw
In reply to Roel Kluin
Quoted 11 lines · Roel Kluin

hdr_delta is unsigned, so take care not to subtract below 0.

Signed-off-by Roel Kluin <[email protected]>

---

net/ipv4/cipso_ipv4.c +0 -0
index 7bc9929..b1d862b 100644
@@ -1999,7 +1999,11 @@ void cipso_v4_sock_delattr(struct sock *sk)
 				iter++;
 		hdr_delta = opt->optlen;

net/ipv4/cipso_ipv4.c void cipso_v4_sock_delattr(struct sock *sk) · line 2009

Quoted 10 lines
net/ipv4/cipso_ipv4.c +5 -1
 		opt->optlen = (optlen_new + 3) & ~3;
-		hdr_delta -= opt->optlen;
+
+		if (hdr_delta > opt->optlen)
+			hdr_delta -= opt->optlen;
+		else
+			hdr_delta = 0;
 	} else {
 		/* only the cipso option was present on the socket so we can
 		 * remove the entire option struct */

This patch isn't needed since I can't think of any sane reason why the hdr_delta subtraction would ever cause an error. Similarly, I don't think David's concerns over the icsk_ext_hdr_len adjustment necessarily apply here either. Let me try and explain, feel free to point out faults in my logic ...

This function, cipso_v4_sock_delattr(), is only used _to_ remove the CIPSO IP option from a socket, never to add it. In the code snippet above we see the following three lines:

	hdr_delta = opt->optlen;
	opt->optlen = (optlen_new + 3) & ~3;
	hdr_delta -= opt->optlen;

The first line sets hdr_delta equal to length of the original IP option list (length includes the CIPSO option). The second line calculates the new length of the IP option list (without the CIPSO option), complete with padding. The third line subtracts the new option length from hdr_delta which is currently equal to original option length. The only way that hdr_delta could ever be in danger of going negative would be if the removal of the CIPSO IP option caused the total IP option length to somehow increase; I just can't think how that would be possible.

Signature · 2 lines
paul moore
linux @ hp