[PATCH] NET: catch signed nla_len() retval in tcf_simp_init()

Previous thread: Linux 2.6.25 by Linus Torvalds on Wednesday, April 16, 2008 - 11:32 pm. (5 messages)

Next thread: [PATCH] cgroup: fix a race condition in manipulating tsk->cg_list by Li Zefan on Wednesday, April 16, 2008 - 11:37 pm. (13 messages)
To: <hadi@...>, <netdev@...>, lkml <linux-kernel@...>
Date: Wednesday, April 16, 2008 - 11:33 pm

'datalen' is unsigned, use 'ret' instead to catch a negative return value.

Signed-off-by: Roel Kluin <12o3l@tiscali.nl>
---
diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
index fbde461..b78d015 100644
--- a/net/sched/act_simple.c
+++ b/net/sched/act_simple.c
@@ -114,9 +114,10 @@ static int tcf_simp_init(struct nlattr *nla, struct nlattr *est,
if (defdata == NULL)
return -EINVAL;

- datalen = nla_len(tb[TCA_DEF_DATA]);
- if (datalen <= 0)
+ ret = nla_len(tb[TCA_DEF_DATA]);
+ if (ret <= 0)
return -EINVAL;
+ datalen = ret;

pc = tcf_hash_check(parm->index, a, bind, &simp_hash_info);
if (!pc) {
--

To: <12o3l@...>
Cc: <hadi@...>, <netdev@...>, <linux-kernel@...>
Date: Thursday, April 17, 2008 - 12:37 am

From: Roel Kluin <12o3l@tiscali.nl>

This clobbers 'ret' which is compared to ACT_P_CREATED
later in the function. If the !pc branch below this code
is not taken, ret must be left at it's initial value of
zero. Now, it will take on some non-zero positive value
which is not correct.

--

To: David Miller <davem@...>
Cc: <12o3l@...>, <hadi@...>, <netdev@...>, <linux-kernel@...>
Date: Thursday, April 17, 2008 - 12:55 am

The change is also unnecessary because the attribute was
already validated and the length can not be less than zero.

This patch changes it to only check for zero length.

Signed-off-by: Patrick McHardy <kaber@trash.net>

To: <kaber@...>
Cc: <12o3l@...>, <hadi@...>, <netdev@...>, <linux-kernel@...>
Date: Friday, April 18, 2008 - 2:20 am

From: Patrick McHardy <kaber@trash.net>

Even though there have been discussions to do other things,
this patch moves things in the forward direction so I have
applied it.

Thanks.

--

To: Patrick McHardy <kaber@...>
Cc: David Miller <davem@...>, <12o3l@...>, <netdev@...>, <linux-kernel@...>
Date: Thursday, April 17, 2008 - 8:58 am

Since act_simple is an academic example:
I think that a better solution is to add TCA_DEF_DATA (which is a
string) to the nla_policy. nla_policy is defined but at the moment it is
not used in the call to nla_parse_nested() - might as well use it.

Roel, would you like to take a crack at that? You will need to define
the max size of the string that TCA_DEF_DATA can hold (if you want to do
it cleanly then define it in include/linux/tc_act/tc_defact.h). This MAX
size will appear in the nla_policy.

cheers,
jamal

--

To: <hadi@...>
Cc: David Miller <davem@...>, <12o3l@...>, <netdev@...>, <linux-kernel@...>
Date: Thursday, April 17, 2008 - 9:10 am

Basic validity checks are always performed. But I agree, better
to provide a good example.
--

Previous thread: Linux 2.6.25 by Linus Torvalds on Wednesday, April 16, 2008 - 11:32 pm. (5 messages)

Next thread: [PATCH] cgroup: fix a race condition in manipulating tsk->cg_list by Li Zefan on Wednesday, April 16, 2008 - 11:37 pm. (13 messages)