* [PATCH v2] ima: fix showing large 'violations' or 'runtime_measurements_count'
@ 2018-10-04 0:01 Eric Biggers
2018-10-04 22:21 ` Mimi Zohar
0 siblings, 1 reply; 6+ messages in thread
From: Eric Biggers @ 2018-10-04 0:01 UTC (permalink / raw)
To: linux-integrity, Mimi Zohar, Dmitry Kasatkin
From: Eric Biggers <ebiggers@google.com>
The 12 character temporary buffer is not necessarily long enough to hold
a 'long' value. Increase it.
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
security/integrity/ima/ima_fs.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index ae9d5c766a3ce..4b50fe9c18edd 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -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);
}
--
2.19.0.605.g01d371f741-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ima: fix showing large 'violations' or 'runtime_measurements_count'
2018-10-04 0:01 [PATCH v2] ima: fix showing large 'violations' or 'runtime_measurements_count' Eric Biggers
@ 2018-10-04 22:21 ` Mimi Zohar
2018-10-04 22:28 ` Eric Biggers
0 siblings, 1 reply; 6+ messages in thread
From: Mimi Zohar @ 2018-10-04 22:21 UTC (permalink / raw)
To: Eric Biggers, linux-integrity, Mimi Zohar, Dmitry Kasatkin
On Wed, 2018-10-03 at 17:01 -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
>
> The 12 character temporary buffer is not necessarily long enough to hold
> a 'long' value. Increase it.
>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
> security/integrity/ima/ima_fs.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index ae9d5c766a3ce..4b50fe9c18edd 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -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];
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
> 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);
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ima: fix showing large 'violations' or 'runtime_measurements_count'
2018-10-04 22:21 ` Mimi Zohar
@ 2018-10-04 22:28 ` Eric Biggers
2018-10-05 13:30 ` Mimi Zohar
0 siblings, 1 reply; 6+ messages in thread
From: Eric Biggers @ 2018-10-04 22:28 UTC (permalink / raw)
To: Mimi Zohar; +Cc: linux-integrity, Mimi Zohar, Dmitry Kasatkin
On Thu, Oct 04, 2018 at 06:21:35PM -0400, Mimi Zohar wrote:
> On Wed, 2018-10-03 at 17:01 -0700, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> >
> > The 12 character temporary buffer is not necessarily long enough to hold
> > a 'long' value. Increase it.
> >
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > ---
> > security/integrity/ima/ima_fs.c | 7 ++++---
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> > index ae9d5c766a3ce..4b50fe9c18edd 100644
> > --- a/security/integrity/ima/ima_fs.c
> > +++ b/security/integrity/ima/ima_fs.c
> > @@ -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];
>
> 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
>
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ima: fix showing large 'violations' or 'runtime_measurements_count'
2018-10-04 22:28 ` Eric Biggers
@ 2018-10-05 13:30 ` Mimi Zohar
2018-10-05 16:57 ` Eric Biggers
0 siblings, 1 reply; 6+ messages in thread
From: Mimi Zohar @ 2018-10-05 13:30 UTC (permalink / raw)
To: Eric Biggers; +Cc: linux-integrity, Mimi Zohar, Dmitry Kasatkin
On Thu, 2018-10-04 at 15:28 -0700, Eric Biggers wrote:
> On Thu, Oct 04, 2018 at 06:21:35PM -0400, Mimi Zohar wrote:
> > On Wed, 2018-10-03 at 17:01 -0700, Eric Biggers wrote:
> > > From: Eric Biggers <ebiggers@google.com>
> > >
> > > The 12 character temporary buffer is not necessarily long enough to hold
> > > a 'long' value. Increase it.
> > >
> > > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > > ---
> > > security/integrity/ima/ima_fs.c | 7 ++++---
> > > 1 file changed, 4 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> > > index ae9d5c766a3ce..4b50fe9c18edd 100644
> > > --- a/security/integrity/ima/ima_fs.c
> > > +++ b/security/integrity/ima/ima_fs.c
> > > @@ -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 */
This comment is useless.
> > > + char tmpbuf[32];
char tmpbuf[32]; /* string size needed for largest long value */
> >
> > 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ima: fix showing large 'violations' or 'runtime_measurements_count'
@ 2018-10-05 16:57 ` Eric Biggers
0 siblings, 0 replies; 6+ messages in thread
From: Eric Biggers @ 2018-10-05 16:57 UTC (permalink / raw)
To: Mimi Zohar; +Cc: linux-integrity, Mimi Zohar, Dmitry Kasatkin
On Fri, Oct 05, 2018 at 09:30:54AM -0400, Mimi Zohar wrote:
> On Thu, 2018-10-04 at 15:28 -0700, Eric Biggers wrote:
> > On Thu, Oct 04, 2018 at 06:21:35PM -0400, Mimi Zohar wrote:
> > > On Wed, 2018-10-03 at 17:01 -0700, Eric Biggers wrote:
> > > > From: Eric Biggers <ebiggers@google.com>
> > > >
> > > > The 12 character temporary buffer is not necessarily long enough to hold
> > > > a 'long' value. Increase it.
> > > >
> > > > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > > > ---
> > > > security/integrity/ima/ima_fs.c | 7 ++++---
> > > > 1 file changed, 4 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> > > > index ae9d5c766a3ce..4b50fe9c18edd 100644
> > > > --- a/security/integrity/ima/ima_fs.c
> > > > +++ b/security/integrity/ima/ima_fs.c
> > > > @@ -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 */
>
> This comment is useless.
>
> > > > + char tmpbuf[32];
>
> char tmpbuf[32]; /* string size needed for largest long value */
>
> > >
> > > 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
>
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ima: fix showing large 'violations' or 'runtime_measurements_count'
@ 2018-10-05 16:57 ` Eric Biggers
0 siblings, 0 replies; 6+ messages in thread
From: Eric Biggers @ 2018-10-05 16:57 UTC (permalink / raw)
To: Mimi Zohar; +Cc: linux-integrity, Mimi Zohar, Dmitry Kasatkin
On Fri, Oct 05, 2018 at 09:30:54AM -0400, Mimi Zohar wrote:
> On Thu, 2018-10-04 at 15:28 -0700, Eric Biggers wrote:
> > On Thu, Oct 04, 2018 at 06:21:35PM -0400, Mimi Zohar wrote:
> > > On Wed, 2018-10-03 at 17:01 -0700, Eric Biggers wrote:
> > > > From: Eric Biggers <ebiggers@google.com>
> > > >
> > > > The 12 character temporary buffer is not necessarily long enough to hold
> > > > a 'long' value. Increase it.
> > > >
> > > > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > > > ---
> > > > security/integrity/ima/ima_fs.c | 7 ++++---
> > > > 1 file changed, 4 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> > > > index ae9d5c766a3ce..4b50fe9c18edd 100644
> > > > --- a/security/integrity/ima/ima_fs.c
> > > > +++ b/security/integrity/ima/ima_fs.c
> > > > @@ -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 */
>
> This comment is useless.
>
> > > > + char tmpbuf[32];
>
> char tmpbuf[32]; /* string size needed for largest long value */
>
> > >
> > > 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
>
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-10-05 23:56 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-04 0:01 [PATCH v2] ima: fix showing large 'violations' or 'runtime_measurements_count' Eric Biggers
2018-10-04 22:21 ` Mimi Zohar
2018-10-04 22:28 ` Eric Biggers
2018-10-05 13:30 ` Mimi Zohar
2018-10-05 16:57 ` Eric Biggers
2018-10-05 16:57 ` Eric Biggers
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.