max cmd boundary chec

hopefully no mime crap
cheers,
jamal

[GENETLINK] max cmd boundary check

We need to boundary check for commands being registered.

---
commit 349e0e00396b79d8f2f9a41f6dc28dee9e7d3e3e
tree 02388c5729f2481644643a41837135bf52698e9e
parent 5465ae68b5ec11b2820db3f9b4c6fd94f113da44
author J Hadi Salim <[email protected]> Fri, 01 Dec 2006 05:59:46 -0500 committer J Hadi Salim <[email protected]> Fri, 01 Dec 2006 05:59:46 -0500

 include/linux/genetlink.h |    1 +
 net/netlink/genetlink.c   |   21 ++++++++++++++-------
 2 files changed, 15 insertions(+), 7 deletions(-)

index 9049dc6..2427d58 100644
@@ -7,6 +7,7 @@
 
 #define GENL_MIN_ID	NLMSG_MIN_TYPE
 #define GENL_MAX_ID	1023
+#define GENL_MAX_CMDS	256
 
 struct genlmsghdr {
 	__u8	cmd;

index cc874f0..50928da 100644
@@ -79,13 +79,22 @@ static struct genl_family *genl_family_find_byname(char *name)
 	return NULL;
 }
 
-static struct genl_ops *genl_get_cmd(u8 cmd, struct genl_family *family)
+static struct genl_ops *genl_get_cmd(u8 cmd, struct genl_family *family, int *err)
 {
+	int i = 0;
 	struct genl_ops *ops;
 
-	list_for_each_entry(ops, &family->ops_list, ops_list)
-		if (ops->cmd == cmd)
+	list_for_each_entry(ops, &family->ops_list, ops_list) {
+		if (ops->cmd == cmd) {
+			*err = -EEXIST;
 			return ops;
+		}
+		if (++i > GENL_MAX_CMDS) {
+			/* is there a better code for exceeding range?*/
+			*err = -ERANGE;
+			return ops;
+		}
+	}
 
 	return NULL;
 }

@@ -138,10 +147,8 @@ int genl_register_ops(struct genl_family *family, struct genl_ops *ops)
 	if (ops->dumpit == NULL && ops->doit == NULL)
 		goto errout;
 
-	if (genl_get_cmd(ops->cmd, family)) {
-		err = -EEXIST;
+	if (genl_get_cmd(ops->cmd, family, &err)) 
 		goto errout;
-	}
 
 	genl_lock();
 	list_add_tail(&ops->ops_list, &family->ops_list);

@@ -313,7 +320,7 @@ static int genl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (nlh->nlmsg_len < nlmsg_msg_size(hdrlen))
 		goto errout;
 
-	ops = genl_get_cmd(hdr->cmd, family);
+	ops = genl_get_cmd(hdr->cmd, family, &err);
 	if (ops == NULL) {
 		err = -EOPNOTSUPP;
 		goto errout;

* jamal <[email protected]> 2006-12-01 06:30

I can't see why this should be required. genl_register_ops() enforces a unique command id and genl_ops->cmd is u8 so there is no way to register more than 256 commands anyway.

Shall i assume that the patch showed up fine i.e no crap like mime? I still didnt get an echo back, did it make the list?

By mistake during the tutorial, i had the id at something like 321. It registered fine but then listing the command showed it with a different id than what i thought it should be. I think it chops off all the bystes other than the LS one - which is not a good error check.
The compiler will whine actually. If you ignore it (perhaps not seeing the warning in a mass compile) it registers just fine.

cheers,
jamal

* jamal <[email protected]> 2006-12-01 09:30

There is no way to fix this in the interface. If you do u8 op = 312 and ignore the compiler warning which states that the value has been truncated it can't be helped, the interface will see op = 56 and register it normally. It is logically impossible to have more than 256 entries on the cmd list, the boundry check you're adding is completely useless.

Indeed I think this is what i saw.
312 == 0x138
This will be chopped to 0x38 == decimal 56

You cannot have more than 256 commands because 0x138 and 0x38 are treated as the same command. So does 0x238, 0x1138...
It is useless/unneeded if the register ops will always see the chopped value. Is this so?

cheers,
jamal

* jamal <[email protected]> 2006-12-01 09:52

The interface enforces a proper value by the type it accepts which is certainly more desireable than a runtime error. If you happen to ignore compiler warnings, then maybe there is the problem. Despite of all this, the patch you propose doesn't work anyway.

If you really want a runtime error you have to change the cmd field in genl_ops to be of larger size and then just check > MAX_ARGS in register_ops() rather than making get_cmd() more expensive which is called for every message received. I don't see any reason why this should be better than a compile warning.

It is easy to overlook compiler warnings. However, it is not an excuse.

Ok, you make a good arguement. Lets just junk this patch.

cheers,
jamal