[G[PATCH 1/2][ENETLINK] max cmd boundary chec

[G[PATCH 1/2][ENETLINK] max cmd boundary chec

[jamal]

[-- Attachment #1: Type: text/plain, Size: 37 bytes --]

hopefully no mime crap
cheers,
jamal

[-- Attachment #2: gnl-dec1-1 --]
[-- Type: text/plain, Size: 2338 bytes --]

[GENETLINK] max cmd boundary check

We need to boundary check for commands being registered.

Signed-off-by: Jamal Hadi Salim<hadi@cyberus.ca>

---
commit 349e0e00396b79d8f2f9a41f6dc28dee9e7d3e3e
tree 02388c5729f2481644643a41837135bf52698e9e
parent 5465ae68b5ec11b2820db3f9b4c6fd94f113da44
author J Hadi Salim <hadi@cyberus.ca> Fri, 01 Dec 2006 05:59:46 -0500
committer J Hadi Salim <hadi@cyberus.ca> 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(-)

diff --git a/include/linux/genetlink.h b/include/linux/genetlink.h
index 9049dc6..2427d58 100644
--- a/include/linux/genetlink.h
+++ b/include/linux/genetlink.h
@@ -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;
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index cc874f0..50928da 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -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;

Re: [G[PATCH 1/2][ENETLINK] max cmd boundary chec

[Thomas Graf]

* jamal <hadi@cyberus.ca> 2006-12-01 06:30
> hopefully no mime crap
> cheers,
> jamal

> [GENETLINK] max cmd boundary check
> 
> We need to boundary check for commands being registered.
> 
> Signed-off-by: Jamal Hadi Salim<hadi@cyberus.ca>

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.

Re: [G[PATCH 1/2][ENETLINK] max cmd boundary chec

[jamal]

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?

On Fri, 2006-01-12 at 13:49 +0100, Thomas Graf wrote:

> 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.

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

Re: [G[PATCH 1/2][ENETLINK] max cmd boundary chec

[Thomas Graf]

* jamal <hadi@cyberus.ca> 2006-12-01 09:30
> 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?
> 
> On Fri, 2006-01-12 at 13:49 +0100, Thomas Graf wrote:
> 
> > 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.
> 
> 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.

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.

Re: [G[PATCH 1/2][ENETLINK] max cmd boundary chec

[jamal]

On Fri, 2006-01-12 at 15:40 +0100, Thomas Graf wrote:

> 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 

Indeed I think this is what i saw. 
312 == 0x138
This will be chopped to 0x38 == decimal 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.

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

Re: [G[PATCH 1/2][ENETLINK] max cmd boundary chec

[Thomas Graf]

* jamal <hadi@cyberus.ca> 2006-12-01 09:52
> 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?

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.

Re: [G[PATCH 1/2][ENETLINK] max cmd boundary chec

[jamal]

On Fri, 2006-01-12 at 16:16 +0100, Thomas Graf wrote:

> 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. 

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

> 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.


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

cheers,
jamal