Skip to contentSkip to conversation

Smack: harmless underflow in smk_set_cipso()

PATCH In review Dan Carpenter 2 participants 3 messages Nov 3, 2015 – Dec 4, 2015

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

PATCH 1/1 Smack: harmless underflow in smk_set_cipso()

This causes a static checker warning because "maplevel" is set by the user and we cap the upper bound but not the lower bound. It seems harmless to me and it's root only but we may as well make the static checker happy.

Also checkpatch complains that we should use kstrtouint() instead of sscanf here.

security/smack/smackfs.c +3 -3
index 94bd9e4..ebb1241 100644
@@ -862,7 +862,7 @@ static ssize_t smk_set_cipso(struct file *file, const char __user *buf,
 	struct smack_known *skp;
 	struct netlbl_lsm_secattr ncats;
 	char mapcatset[SMK_CIPSOLEN];
-	int maplevel;
+	unsigned int maplevel;
 	unsigned int cat;
 	int catlen;
 	ssize_t rc = -EINVAL;
@@ -912,8 +912,8 @@ static ssize_t smk_set_cipso(struct file *file, const char __user *buf,
 	else
 		rule += strlen(skp->smk_known) + 1;
 
-	ret = sscanf(rule, "%d", &maplevel);
-	if (ret != 1 || maplevel > SMACK_CIPSO_MAXLEVEL)
+	ret = kstrtouint(rule, 10, &maplevel);
+	if (ret || maplevel > SMACK_CIPSO_MAXLEVEL)
 		goto out;
 
 	rule += SMK_DIGITLEN;
maintainer
#1 lore raw
Quoted 9 lines · Dan Carpenter

This causes a static checker warning because "maplevel" is set by the user and we cap the upper bound but not the lower bound. It seems harmless to me and it's root only but we may as well make the static checker happy.

Also checkpatch complains that we should use kstrtouint() instead of sscanf here.

This no longer parses cipso specifications correctly. I haven't investigated why. It looks as if it should work. Does kstrtouint() allow for leading whitespace? If not, that's the problem.

Quoted 26 lines
security/smack/smackfs.c +3 -3
index 94bd9e4..ebb1241 100644
@@ -862,7 +862,7 @@ static ssize_t smk_set_cipso(struct file *file, const char __user *buf,
  	struct smack_known *skp;
  	struct netlbl_lsm_secattr ncats;
  	char mapcatset[SMK_CIPSOLEN];
-	int maplevel;
+	unsigned int maplevel;
  	unsigned int cat;
  	int catlen;
  	ssize_t rc = -EINVAL;
@@ -912,8 +912,8 @@ static ssize_t smk_set_cipso(struct file *file, const char __user *buf,
  	else
  		rule += strlen(skp->smk_known) + 1;
 
-	ret = sscanf(rule, "%d", &maplevel);
-	if (ret != 1 || maplevel > SMACK_CIPSO_MAXLEVEL)
+	ret = kstrtouint(rule, 10, &maplevel);
+	if (ret || maplevel > SMACK_CIPSO_MAXLEVEL)
  		goto out;
 
  	rule += SMK_DIGITLEN;
#2 lore raw
In reply to Dan Carpenter
Quoted 9 lines · Casey Schaufler
Quoted 4 lines · Dan Carpenter

Also checkpatch complains that we should use kstrtouint() instead of sscanf here.

This no longer parses cipso specifications correctly. I haven't investigated why. It looks as if it should work. Does kstrtouint() allow for leading whitespace?

No it doesn't. I feel slightly betrayed because checkpatch.pl told me to introduce that bug. I have said so many times that I think checkpatch.pl should not tell people to introduce bugs...

regards,
dan carpenter