ima: fix showing large 'violations' or 'runtime_measurements_count'

PATCH In review Eric Biggers 2 participants 5 messages Oct 4, 2018 – Oct 5, 2018

Subsystems (4)

Security (LSM) Keys-trusted Integrity measurement architecture (IMA) Extended Verification Module (EVM)

lore

Share & tools

lore

Eric Biggers

author

2018-10-04 06:52 UTC #0 lore raw

PATCH v2 1/1 ima: fix showing large 'violations' or 'runtime_measurements_count'

From: Eric Biggers <[email protected]>

The 12 character temporary buffer is not necessarily long enough to hold a 'long' value. Increase it.

Signed-off-by Eric Biggers <[email protected]>

---
 security/integrity/ima/ima_fs.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

security/integrity/ima/ima_fs.c +4 -3

index ae9d5c766a3ce..4b50fe9c18edd 100644
@@ -42,14 +42,15 @@ static int __init default_canonical_fmt_setup(char *str)
 __setup("ima_canonical_fmt", default_canonical_fmt_setup);
 
 static int valid_policy = 1;
-#define TMPBUFLEN 12
+
 static ssize_t ima_show_htable_value(char __user *buf, size_t count,
 				     loff_t *ppos, atomic_long_t *val)
 {
-	char tmpbuf[TMPBUFLEN];
+	/* temporary buffer that is plenty long enough */
+	char tmpbuf[32];
 	ssize_t len;
 
-	len = scnprintf(tmpbuf, TMPBUFLEN, "%li\n", atomic_long_read(val));
+	len = scnprintf(tmpbuf, sizeof(tmpbuf), "%li\n", atomic_long_read(val));
 	return simple_read_from_buffer(buf, count, ppos, tmpbuf, len);
 }

Signature · 1 line

2.19.0.605.g01d371f741-goog

Mimi Zohar

maintainer

2018-10-05 05:17 UTC #1 lore raw

Quoted 16 lines · Eric Biggers

From: Eric Biggers <[email protected]>
The 12 character temporary buffer is not necessarily long enough to hold a 'long' value. Increase it.
Signed-off-by Eric Biggers <[email protected]>
---
 security/integrity/ima/ima_fs.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
security/integrity/ima/ima_fs.c +0 -0
index ae9d5c766a3ce..4b50fe9c18edd 100644
@@ -42,14 +42,15 @@ static int __init default_canonical_fmt_setup(char *str)
 __setup("ima_canonical_fmt", default_canonical_fmt_setup);

security/integrity/ima/ima_fs.c static int __init default_canonical_fmt_setup(char *str) · line 50

Quoted 10 lines

security/integrity/ima/ima_fs.c +3 -2

 
 static int valid_policy = 1;
-#define TMPBUFLEN 12
+
 static ssize_t ima_show_htable_value(char __user *buf, size_t count,
 				     loff_t *ppos, atomic_long_t *val)
 {
-	char tmpbuf[TMPBUFLEN];
+	/* temporary buffer that is plenty long enough */
+	char tmpbuf[32];

If the maximum value of long is 9,223,372,036,854,775,807, the largest string needed to represent this value is 20 characters. Should 32 be hardcoded like this?

Mimi

Quoted 7 lines

security/integrity/ima/ima_fs.c +1 -1

 	ssize_t len;
 
-	len = scnprintf(tmpbuf, TMPBUFLEN, "%li\n", atomic_long_read(val));
+	len = scnprintf(tmpbuf, sizeof(tmpbuf), "%li\n", atomic_long_read(val));
 	return simple_read_from_buffer(buf, count, ppos, tmpbuf, len);
 }

Eric Biggers

author

2018-10-05 05:24 UTC #2 lore raw

Quoted 32 lines · Mimi Zohar

Quoted 16 lines · Eric Biggers
From: Eric Biggers <[email protected]>
The 12 character temporary buffer is not necessarily long enough to hold a 'long' value. Increase it.
Signed-off-by Eric Biggers <[email protected]>
---
 security/integrity/ima/ima_fs.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
security/integrity/ima/ima_fs.c +0 -0
index ae9d5c766a3ce..4b50fe9c18edd 100644
@@ -42,14 +42,15 @@ static int __init default_canonical_fmt_setup(char *str)
 __setup("ima_canonical_fmt", default_canonical_fmt_setup);
security/integrity/ima/ima_fs.c static int __init default_canonical_fmt_setup(char *str) · line 50
Quoted 10 lines
security/integrity/ima/ima_fs.c +3 -2
 
 static int valid_policy = 1;
-#define TMPBUFLEN 12
+
 static ssize_t ima_show_htable_value(char __user *buf, size_t count,
 				     loff_t *ppos, atomic_long_t *val)
 {
-	char tmpbuf[TMPBUFLEN];
+	/* temporary buffer that is plenty long enough */
+	char tmpbuf[32];
If the maximum value of long is 9,223,372,036,854,775,807, the largest string needed to represent this value is 20 characters. Should 32 be hardcoded like this?

Quoted 2 lines

Mimi

There's no real cost to overestimating slightly here, and it's better than trying to count exactly and getting it wrong (hint: it's actually more than 20 characters).

- Eric

Mimi Zohar

maintainer

2018-10-05 20:29 UTC #3 lore raw

Quoted 19 lines · Eric Biggers

Quoted 18 lines · Mimi Zohar
Quoted 17 lines · Eric Biggers
From: Eric Biggers <[email protected]>
The 12 character temporary buffer is not necessarily long enough to hold a 'long' value. Increase it.
Signed-off-by Eric Biggers <[email protected]>
---
 security/integrity/ima/ima_fs.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
security/integrity/ima/ima_fs.c +0 -0
index ae9d5c766a3ce..4b50fe9c18edd 100644
@@ -42,14 +42,15 @@ static int __init default_canonical_fmt_setup(char *str)
 __setup("ima_canonical_fmt", default_canonical_fmt_setup);
 

Quoted 8 lines

Quoted 8 lines
security/integrity/ima/ima_fs.c static int __init default_canonical_fmt_setup(char *str) · line 49
Quoted 8 lines
security/integrity/ima/ima_fs.c +2 -2
 static int valid_policy = 1;
-#define TMPBUFLEN 12
+
 static ssize_t ima_show_htable_value(char __user *buf, size_t count,
 				     loff_t *ppos, atomic_long_t *val)
 {
-	char tmpbuf[TMPBUFLEN];
+	/* temporary buffer that is plenty long enough */

This comment is useless.

Quoted 1 line

Quoted 1 line
security/integrity/ima/ima_fs.c static int __init default_canonical_fmt_setup(char *str) · line 50
Quoted 1 line
security/integrity/ima/ima_fs.c +1 -0
+	char tmpbuf[32];

char tmpbuf[32]; /* string size needed for largest long value */

Quoted 8 lines

Quoted 4 lines
If the maximum value of long is 9,223,372,036,854,775,807, the largest string needed to represent this value is 20 characters. Should 32 be hardcoded like this?
There's no real cost to overestimating slightly here, and it's better than trying to count exactly and getting it wrong (hint: it's actually more than 20 characters).

Please explain how it is more than 20 characters.

Mimi

Eric Biggers

author

2018-10-05 16:57 UTC #4 lore raw

Quoted 40 lines · Mimi Zohar

Quoted 19 lines · Eric Biggers
Quoted 18 lines · Mimi Zohar
Quoted 17 lines · Eric Biggers
From: Eric Biggers <[email protected]>
The 12 character temporary buffer is not necessarily long enough to hold a 'long' value. Increase it.
Signed-off-by Eric Biggers <[email protected]>
---
 security/integrity/ima/ima_fs.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
security/integrity/ima/ima_fs.c +0 -0
index ae9d5c766a3ce..4b50fe9c18edd 100644
@@ -42,14 +42,15 @@ static int __init default_canonical_fmt_setup(char *str)
 __setup("ima_canonical_fmt", default_canonical_fmt_setup);
 
Quoted 8 lines
Quoted 8 lines
security/integrity/ima/ima_fs.c static int __init default_canonical_fmt_setup(char *str) · line 49
Quoted 8 lines
security/integrity/ima/ima_fs.c +2 -2
 static int valid_policy = 1;
-#define TMPBUFLEN 12
+
 static ssize_t ima_show_htable_value(char __user *buf, size_t count,
 				     loff_t *ppos, atomic_long_t *val)
 {
-	char tmpbuf[TMPBUFLEN];
+	/* temporary buffer that is plenty long enough */
This comment is useless.
Quoted 1 line
Quoted 1 line
security/integrity/ima/ima_fs.c static int __init default_canonical_fmt_setup(char *str) · line 50
Quoted 1 line
security/integrity/ima/ima_fs.c +1 -0
+	char tmpbuf[32];
char tmpbuf[32]; /* string size needed for largest long value */
Quoted 5 lines
Quoted 4 lines
If the maximum value of long is 9,223,372,036,854,775,807, the largest string needed to represent this value is 20 characters. Should 32 be hardcoded like this?

Quoted 8 lines

Quoted 3 lines
There's no real cost to overestimating slightly here, and it's better than trying to count exactly and getting it wrong (hint: it's actually more than 20 characters).
Please explain how it is more than 20 characters.
Mimi

sizeof("-9223372036854775808\n") == 22.

Yes the comment is pretty useless, that's why I didn't want to have a comment; but you requested one. Personally, I'm not very interested in arguing about the wording of an unnecessary comment (especially when your suggested wording is misleading) and having to explain how to count characters in a string. So just FYI, I won't be sending any more versions of this patch; if you don't like either of the versions I've sent, please just write your own.

- Eric