Skip to contentSkip to conversation

netlink: fix a limit in NETLINK_LIST_MEMBERSHIPS

PATCH In review Dan Carpenter 2 participants 3 messages Nov 13, 2015

Subsystems (3)
Networking Network drivers Kernel janitors
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 netlink: fix a limit in NETLINK_LIST_MEMBERSHIPS

This condition doesn't work when len is smaller than expected and not a multiple of 4. In that situation "len - pos" is negative and type promoted to a high unsigned value and we do not break out of the loop. It causes the program calling it to crash.

Fixes b42be38b2778 ('netlink: add API to retrieve all group memberships')
Signed-off-by Dan Carpenter <[email protected]>
net/netlink/af_netlink.c +1 -1
index 59651af..76a8466 100644
@@ -2373,7 +2373,7 @@ static int netlink_getsockopt(struct socket *sock, int level, int optname,
 		err = 0;
 		netlink_lock_table();
 		for (pos = 0; pos * 8 < nlk->ngroups; pos += sizeof(u32)) {
-			if (len - pos < sizeof(u32))
+			if (len < pos + sizeof(u32))
 				break;
 
 			idx = pos / sizeof(unsigned long);
#1 lore raw

Hi

Quoted 4 lines · Dan Carpenter

This condition doesn't work when len is smaller than expected and not a multiple of 4. In that situation "len - pos" is negative and type promoted to a high unsigned value and we do not break out of the loop. It causes the program calling it to crash.

Could you give an example how this can happen? The loop-invariant should be "len >= pos", as such, this shouldn't happen. "pos" starts out as 0, "len" is guaranteed to be >=0. "pos" is only incremented by 4, if "len - pos >= 4".

What am I missing?

Thanks
David

Quoted 16 lines
Fixes b42be38b2778 ('netlink: add API to retrieve all group memberships')
Signed-off-by Dan Carpenter <[email protected]>
net/netlink/af_netlink.c +1 -1
index 59651af..76a8466 100644
@@ -2373,7 +2373,7 @@ static int netlink_getsockopt(struct socket *sock, int level, int optname,
                err = 0;
                netlink_lock_table();
                for (pos = 0; pos * 8 < nlk->ngroups; pos += sizeof(u32)) {
-                       if (len - pos < sizeof(u32))
+                       if (len < pos + sizeof(u32))
                                break;
 
                        idx = pos / sizeof(unsigned long);
author
#2 lore raw

Oh. Crap... My mistake. Sorry for the noise. The original code is fine.

regards,
dan carpenter