Skip to contentSkip to conversation

apparmor: secid: fix error return value in error handling path

PATCH In review Gustavo A. R. Silva 2 participants 3 messages May 4, 2018 – May 15, 2018

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH v2 1/1 apparmor: secid: fix error return value in error handling path

Currently, function apparmor_secid_to_secctx returns always zero, no matter if the value returned by aa_label_asxprint is negative (which implies that an error has occurred).

Notice that *seclen* is of type u32 (32 bits, unsigned), and a less-than-zero comparison of an unsigned value is never true.

Fix this by temporarily storing the value returned by aa_label_asxprint into a variable of type int (signed) for its further evaluation.

Addresses-Coverity-ID: 1468514 ("Unsigned compared against 0")

Fixes c092921219d2 ("apparmor: add support for mapping secids and using

secctxes")

---
Changes is v2:
- Update changelog.
- Fix misaligned lines.

 security/apparmor/secid.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)
security/apparmor/secid.c +14 -9
index 5029248..efaa0d9 100644
@@ -142,6 +142,7 @@ int apparmor_secid_to_secctx(u32 secid, char **secdata, u32 *seclen)
 {
 	/* TODO: cache secctx and ref count so we don't have to recreate */
 	struct aa_label *label = aa_secid_to_label(secid);
+	int seclen_tmp;
 
 	AA_BUG(!secdata);
 	AA_BUG(!seclen);
@@ -150,17 +151,21 @@ int apparmor_secid_to_secctx(u32 secid, char **secdata, u32 *seclen)
 		return -EINVAL;
 
 	if (secdata)
-		*seclen = aa_label_asxprint(secdata, root_ns, label,
-					    FLAG_SHOW_MODE | FLAG_VIEW_SUBNS |
-					    FLAG_HIDDEN_UNCONFINED |
-					    FLAG_ABS_ROOT, GFP_ATOMIC);
+		seclen_tmp = aa_label_asxprint(secdata, root_ns, label,
+					       FLAG_SHOW_MODE |
+					       FLAG_VIEW_SUBNS |
+					       FLAG_HIDDEN_UNCONFINED |
+					       FLAG_ABS_ROOT, GFP_ATOMIC);
 	else
-		*seclen = aa_label_snxprint(NULL, 0, root_ns, label,
-					    FLAG_SHOW_MODE | FLAG_VIEW_SUBNS |
-					    FLAG_HIDDEN_UNCONFINED |
-					    FLAG_ABS_ROOT);
-	if (*seclen < 0)
+		seclen_tmp = aa_label_snxprint(NULL, 0, root_ns, label,
+					       FLAG_SHOW_MODE |
+					       FLAG_VIEW_SUBNS |
+					       FLAG_HIDDEN_UNCONFINED |
+					       FLAG_ABS_ROOT);
+	if (seclen_tmp < 0)
 		return -ENOMEM;
+	else
+		*seclen = seclen_tmp;
 
 	return 0;
 }
Signature · 1 line
2.7.4
#1 lore raw

Hi Gustavo,

if you are using the coverity scan bug tracking, please can you mark a bug as being worked on by yourself so I don't work on it at the same time as we're duplicating work here.

Colin

Quoted 66 lines · Gustavo A. R. Silva

Currently, function apparmor_secid_to_secctx returns always zero, no matter if the value returned by aa_label_asxprint is negative (which implies that an error has occurred).

Notice that *seclen* is of type u32 (32 bits, unsigned), and a less-than-zero comparison of an unsigned value is never true.

Fix this by temporarily storing the value returned by aa_label_asxprint into a variable of type int (signed) for its further evaluation.

Addresses-Coverity-ID: 1468514 ("Unsigned compared against 0")

Fixes c092921219d2 ("apparmor: add support for mapping secids and using

secctxes")

---
Changes is v2:
- Update changelog.
- Fix misaligned lines.

 security/apparmor/secid.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)
security/apparmor/secid.c +14 -9
index 5029248..efaa0d9 100644
@@ -142,6 +142,7 @@ int apparmor_secid_to_secctx(u32 secid, char **secdata, u32 *seclen)
 {
 	/* TODO: cache secctx and ref count so we don't have to recreate */
 	struct aa_label *label = aa_secid_to_label(secid);
+	int seclen_tmp;
 
 	AA_BUG(!secdata);
 	AA_BUG(!seclen);
@@ -150,17 +151,21 @@ int apparmor_secid_to_secctx(u32 secid, char **secdata, u32 *seclen)
 		return -EINVAL;
 
 	if (secdata)
-		*seclen = aa_label_asxprint(secdata, root_ns, label,
-					    FLAG_SHOW_MODE | FLAG_VIEW_SUBNS |
-					    FLAG_HIDDEN_UNCONFINED |
-					    FLAG_ABS_ROOT, GFP_ATOMIC);
+		seclen_tmp = aa_label_asxprint(secdata, root_ns, label,
+					       FLAG_SHOW_MODE |
+					       FLAG_VIEW_SUBNS |
+					       FLAG_HIDDEN_UNCONFINED |
+					       FLAG_ABS_ROOT, GFP_ATOMIC);
 	else
-		*seclen = aa_label_snxprint(NULL, 0, root_ns, label,
-					    FLAG_SHOW_MODE | FLAG_VIEW_SUBNS |
-					    FLAG_HIDDEN_UNCONFINED |
-					    FLAG_ABS_ROOT);
-	if (*seclen < 0)
+		seclen_tmp = aa_label_snxprint(NULL, 0, root_ns, label,
+					       FLAG_SHOW_MODE |
+					       FLAG_VIEW_SUBNS |
+					       FLAG_HIDDEN_UNCONFINED |
+					       FLAG_ABS_ROOT);
+	if (seclen_tmp < 0)
 		return -ENOMEM;
+	else
+		*seclen = seclen_tmp;
 
 	return 0;
 }
#2 lore raw

Hi Colin,

Sorry for the late response. I've been on the road for the last 12 days and I'm just back.

Quoted 6 lines · Colin Ian King

Hi Gustavo,

if you are using the coverity scan bug tracking, please can you mark a bug as being worked on by yourself so I don't work on it at the same time as we're duplicating work here.

Yeah, I've noticed that. I actually do that very often, and in some cases I've also noticed you have made changes to the same bug regardless of that.

The problem is that the Coverity dashboard doesn't refresh itself, so anyone using it should refresh it at all times in order to keep track of the bugs that have already been taken by somebody else.

So I think this kind of "collisions" will continue to happen unless people add the auto-refresh feature to the Coverity dashboard. :/

Thanks
--
Gustavo