Skip to contentSkip to conversation

NET: catch signed nla_len() retval in tcf_simp_init()

PATCH In review Roel Kluin 4 participants 7 messages Apr 17, 2008 – Apr 18, 2008

Subsystems (3)
Networking Network drivers Traffic control (tc)
lore
Share & tools
lore

Roel Kluin

author
#0 lore raw

PATCH 1/1 NET: catch signed nla_len() retval in tcf_simp_init()

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

Signed-off-by Roel Kluin <[email protected]>

---

net/sched/act_simple.c +3 -2
index fbde461..b78d015 100644
@@ -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) {
maintainer
#1 lore raw

From: Roel Kluin <[email protected]>
Date: Thu, 17 Apr 2008 05:33:21 +0200

Quoted 11 lines

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

Signed-off-by Roel Kluin <[email protected]>

---

net/sched/act_simple.c +0 -0
index fbde461..b78d015 100644
@@ -114,9 +114,10 @@ static int tcf_simp_init(struct nlattr *nla, struct nlattr *est,
 	if (defdata == NULL)
 		return -EINVAL;

net/sched/act_simple.c static int tcf_simp_init(struct nlattr *nla, struct nlattr *est, · line 123

Quoted 10 lines
net/sched/act_simple.c +3 -2
 
-	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) {

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.

#2 lore raw
Quoted 22 lines · David Miller

From: Roel Kluin <[email protected]>
Date: Thu, 17 Apr 2008 05:33:21 +0200

Quoted 19 lines

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

Signed-off-by Roel Kluin <[email protected]>

---

net/sched/act_simple.c +3 -2
index fbde461..b78d015 100644
@@ -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;
Quoted 8 lines

net/sched/act_simple.c static int tcf_simp_init(struct nlattr *nla, struct nlattr *est, · line 123

Quoted 2 lines
net/sched/act_simple.c +0 -0
 	pc = tcf_hash_check(parm->index, a, bind, &simp_hash_info);
 	if (!pc) {

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.

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 <[email protected]>
net/sched/act_simple.c +1 -1
index fbde461..64b2d13 100644
@@ -115,7 +115,7 @@ static int tcf_simp_init(struct nlattr *nla, struct nlattr *est,
 		return -EINVAL;
 
 	datalen = nla_len(tb[TCA_DEF_DATA]);
-	if (datalen <= 0)
+	if (datalen == 0)
 		return -EINVAL;
 
 	pc = tcf_hash_check(parm->index, a, bind, &simp_hash_info);
#3 lore raw
Quoted 2 lines · Patrick McHardy

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

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

#4 lore raw
Quoted 9 lines · jamal
Quoted 2 lines · Patrick McHardy

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

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.

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

#5 lore raw
Quoted 2 lines · Patrick McHardy

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

Thanks Patrick.
I just noticed the iproute2 piece is missing - probably sitting somewhere in my old laptop if i can find it.
Roel, you wanna take a crack at that too?

cheers,
jamal

maintainer
#6 lore raw
In reply to Patrick McHardy

From: Patrick McHardy <[email protected]>
Date: Thu, 17 Apr 2008 06:55:35 +0200

Quoted 6 lines

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 <[email protected]>

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

Thanks.