Skip to contentSkip to conversation

netlink: fix (theoretical) overrun in message iteration

PATCH In review Vegard Nossum 2 participants 4 messages Dec 21, 2008 – Dec 26, 2008

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Vegard Nossum

author
#0 lore raw

PATCH 1/1 netlink: fix (theoretical) overrun in message iteration

Quoted 1 line

From bb805d89e84ddb11c9bb58afcfd9a6b37bbe5a9b Mon Sep 17 00:00:00 2001

From: Vegard Nossum <[email protected]>
Date: Sun, 21 Dec 2008 14:20:49 +0100
Subject: [PATCH] netlink: fix (theoretical) overrun in message iteration

See commit 1045b03e07d85f3545118510a587035536030c1c for a detailed explanation of why this patch is necessary.

In short, nlmsg_next() can make "remaining" go negative, and the remaining >= sizeof(...) comparison will promote "remaining" to an unsigned type, which means that the expression will evaluate to true for negative numbers, even though it was not intended.

I put "theoretical" in the title because I have no evidence that this can actually happen, but I suspect that a crafted netlink packet can trigger some badness.

Note that the last test, which seemingly has the exact same problem (also true for nla_ok()), is perfectly OK, since we already know that remaining is positive.

Cc Thomas Graf <[email protected]>
Signed-off-by Vegard Nossum <[email protected]>
---
 include/net/netlink.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
include/net/netlink.h +1 -1
index 3643bbb..13dd525 100644
@@ -332,7 +332,7 @@ static inline int nlmsg_attrlen(const struct nlmsghdr *nlh, int hdrlen)
  */
 static inline int nlmsg_ok(const struct nlmsghdr *nlh, int remaining)
 {
-	return (remaining >= sizeof(struct nlmsghdr) &&
+	return (remaining >= (int) sizeof(struct nlmsghdr) &&
 		nlh->nlmsg_len >= sizeof(struct nlmsghdr) &&
 		nlh->nlmsg_len <= remaining);
 }
Signature · 1 line
1.5.6.5
author
#1 lore raw
Quoted 8 lines · Vegard Nossum

From bb805d89e84ddb11c9bb58afcfd9a6b37bbe5a9b Mon Sep 17 00:00:00 2001
From: Vegard Nossum <[email protected]>
Date: Sun, 21 Dec 2008 14:20:49 +0100
Subject: [PATCH] netlink: fix (theoretical) overrun in message iteration

See commit 1045b03e07d85f3545118510a587035536030c1c for a detailed explanation of why this patch is necessary.

Quoted 8 lines

In short, nlmsg_next() can make "remaining" go negative, and the remaining >= sizeof(...) comparison will promote "remaining" to an unsigned type, which means that the expression will evaluate to true for negative numbers, even though it was not intended.

I put "theoretical" in the title because I have no evidence that this can actually happen, but I suspect that a crafted netlink packet can trigger some badness.

nlmsg

Signature · 4 lines
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036
author
#2 lore raw
Quoted 14 lines · Vegard Nossum
Quoted 13 lines · Vegard Nossum

From bb805d89e84ddb11c9bb58afcfd9a6b37bbe5a9b Mon Sep 17 00:00:00 2001
From: Vegard Nossum <[email protected]>
Date: Sun, 21 Dec 2008 14:20:49 +0100
Subject: [PATCH] netlink: fix (theoretical) overrun in message iteration

See commit 1045b03e07d85f3545118510a587035536030c1c for a detailed explanation of why this patch is necessary.

In short, nlmsg_next() can make "remaining" go negative, and the remaining >= sizeof(...) comparison will promote "remaining" to an unsigned type, which means that the expression will evaluate to true for negative numbers, even though it was not intended.

Quoted 5 lines
Quoted 3 lines

I put "theoretical" in the title because I have no evidence that this can actually happen, but I suspect that a crafted netlink packet can trigger some badness.

nlmsg

Oops. I meant to say that nlmsg_for_each_msg() has no users at all, which means that the change is all the more "theoretical" :-)

Vegard

Signature · 4 lines
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036
maintainer
#3 lore raw
In reply to Vegard Nossum

From: Vegard Nossum <[email protected]>
Date: Sun, 21 Dec 2008 14:42:18 +0100

Quoted 1 line

netlink: fix (theoretical) overrun in message iteration

I'll apply this but we should also consider getting rid of unused interfaces such as nlmsg_for_each_msg().