2:43 p.m.
Just some updates from the review that I have gathered.
1. Possibility of accessing invalid memory on res being 0.
2. Do not switch ordering of fields.
Does anyone hate using the boolean expression to get my
increment value? Removes some branches. Based on my
loose understanding that boolean operators are guarenteed
to be 0 or 1. Although I am never a big fan of stuff like
this in general. Should I just do the branches?
http://c-faq.com/bool/bool2.html
[PATCH] squash: review-updates
2:44 p.m.
New subject: [PATCH] squash: review-updates
Signed-off-by: William Roberts <wroberts(a)tresys.com>
---
kernel/auditsc.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index a4c2003..9ba1f2a 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1292,9 +1292,20 @@ static void audit_log_cmdline(struct audit_buffer *ab, struct
task_struct *tsk,
if (!buf)
goto out;
res = get_cmdline(tsk, buf, PATH_MAX);
- /* Ensure NULL terminated */
- if (buf[res-1] != '\0')
- buf[res-1] = '\0';
+ if (res == 0) {
+ kfree(buf);
+ goto out;
+ }
+ /*
+ * Ensure NULL terminated but don't clobber the end
+ * unless the buffer is full. Worst case you end up
+ * with 2 null bytes ending it. By doing it this way
+ * one avoids additional branching. One checking if the
+ * end is null and another to check if their should be
+ * an increment before setting the null byte.
+ */
+ res += res < PATH_MAX;
+ buf[res-1] = '\0';
context->cmdline = buf;
}
msg = context->cmdline;
@@ -1333,8 +1344,8 @@ static void audit_log_exit(struct audit_context *context, struct
task_struct *ts
context->name_count);
audit_log_task_info(ab, tsk);
- audit_log_cmdline(ab, tsk, context);
audit_log_key(ab, context->filterkey);
+ audit_log_cmdline(ab, tsk, context);
audit_log_end(ab);
for (aux = context->aux; aux; aux = aux->next) {
--
1.7.9.5
10:01 a.m.
New subject: [PATCH] squash: review-updates
On Tue, Jan 7, 2014 at 12:44 PM, William Roberts
<bill.c.roberts(a)gmail.com> wrote:
Signed-off-by: William Roberts <wroberts(a)tresys.com>
---
kernel/auditsc.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index a4c2003..9ba1f2a 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1292,9 +1292,20 @@ static void audit_log_cmdline(struct audit_buffer *ab, struct
task_struct *tsk,
if (!buf)
goto out;
res = get_cmdline(tsk, buf, PATH_MAX);
- /* Ensure NULL terminated */
- if (buf[res-1] != '\0')
- buf[res-1] = '\0';
+ if (res == 0) {
+ kfree(buf);
+ goto out;
+ }
+ /*
+ * Ensure NULL terminated but don't clobber the end
+ * unless the buffer is full. Worst case you end up
+ * with 2 null bytes ending it. By doing it this way
+ * one avoids additional branching. One checking if the
+ * end is null and another to check if their should be
+ * an increment before setting the null byte.
+ */
+ res += res < PATH_MAX;
+ buf[res-1] = '\0';
context->cmdline = buf;
}
msg = context->cmdline;
@@ -1333,8 +1344,8 @@ static void audit_log_exit(struct audit_context *context, struct
task_struct *ts
context->name_count);
audit_log_task_info(ab, tsk);
- audit_log_cmdline(ab, tsk, context);
audit_log_key(ab, context->filterkey);
+ audit_log_cmdline(ab, tsk, context);
audit_log_end(ab);
for (aux = context->aux; aux; aux = aux->next) {
--
1.7.9.5
FYI for those wondering. Page 205 and 206, Sections A7.9 and A7.10 in
the K+R C book 2nd edition explicitly states that relational and
equality operators return a 0 or 1.
--
Respectfully,
William C Roberts
10:10 a.m.
New subject: [PATCH] squash: review-updates
On 14/01/07, William Roberts wrote:
Signed-off-by: William Roberts <wroberts(a)tresys.com>
---
kernel/auditsc.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index a4c2003..9ba1f2a 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1292,9 +1292,20 @@ static void audit_log_cmdline(struct audit_buffer *ab, struct
task_struct *tsk,
if (!buf)
goto out;
res = get_cmdline(tsk, buf, PATH_MAX);
- /* Ensure NULL terminated */
- if (buf[res-1] != '\0')
- buf[res-1] = '\0';
+ if (res == 0) {
+ kfree(buf);
+ goto out;
+ }
+ /*
+ * Ensure NULL terminated but don't clobber the end
+ * unless the buffer is full. Worst case you end up
+ * with 2 null bytes ending it. By doing it this way
+ * one avoids additional branching. One checking if the
+ * end is null and another to check if their should be
+ * an increment before setting the null byte.
+ */
+ res += res < PATH_MAX;
+ buf[res-1] = '\0';
context->cmdline = buf;
}
msg = context->cmdline;
@@ -1333,8 +1344,8 @@ static void audit_log_exit(struct audit_context *context, struct
task_struct *ts
context->name_count);
audit_log_task_info(ab, tsk);
- audit_log_cmdline(ab, tsk, context);
audit_log_key(ab, context->filterkey);
+ audit_log_cmdline(ab, tsk, context);
audit_log_end(ab);
for (aux = context->aux; aux; aux = aux->next) {
I'm fine with this.
- RGB
--
Richard Guy Briggs <rbriggs(a)redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
3:08 p.m.
New subject: [PATCH] squash: review-updates
If you know the buf len, you can just use audit_log_n_untrusted_string()
I think....
On Tue, 2014-01-07 at 12:44 -0800, William Roberts wrote:
Signed-off-by: William Roberts <wroberts(a)tresys.com>
---
kernel/auditsc.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index a4c2003..9ba1f2a 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1292,9 +1292,20 @@ static void audit_log_cmdline(struct audit_buffer *ab, struct
task_struct *tsk,
if (!buf)
goto out;
res = get_cmdline(tsk, buf, PATH_MAX);
- /* Ensure NULL terminated */
- if (buf[res-1] != '\0')
- buf[res-1] = '\0';
+ if (res == 0) {
+ kfree(buf);
+ goto out;
+ }
+ /*
+ * Ensure NULL terminated but don't clobber the end
+ * unless the buffer is full. Worst case you end up
+ * with 2 null bytes ending it. By doing it this way
+ * one avoids additional branching. One checking if the
+ * end is null and another to check if their should be
+ * an increment before setting the null byte.
+ */
+ res += res < PATH_MAX;
+ buf[res-1] = '\0';
context->cmdline = buf;
}
msg = context->cmdline;
@@ -1333,8 +1344,8 @@ static void audit_log_exit(struct audit_context *context, struct
task_struct *ts
context->name_count);
audit_log_task_info(ab, tsk);
- audit_log_cmdline(ab, tsk, context);
audit_log_key(ab, context->filterkey);
+ audit_log_cmdline(ab, tsk, context);
audit_log_end(ab);
for (aux = context->aux; aux; aux = aux->next) {
3:33 p.m.
New subject: [PATCH] squash: review-updates
On Fri, 2014-01-10 at 16:08 -0500, Eric Paris wrote:
If you know the buf len, you can just use
audit_log_n_untrusted_string()
I think....
admittedly audit_log_n_untrusted_string() will log ALL of n. So you
could use strnlen() as the argument to audit_log_n_untrusted_string()
(or create another helper, I don't care)
-Eric
On Tue, 2014-01-07 at 12:44 -0800, William Roberts wrote:
> Signed-off-by: William Roberts <wroberts(a)tresys.com>
> ---
> kernel/auditsc.c | 19 +++++++++++++++----
> 1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index a4c2003..9ba1f2a 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1292,9 +1292,20 @@ static void audit_log_cmdline(struct audit_buffer *ab, struct
task_struct *tsk,
> if (!buf)
> goto out;
> res = get_cmdline(tsk, buf, PATH_MAX);
> - /* Ensure NULL terminated */
> - if (buf[res-1] != '\0')
> - buf[res-1] = '\0';
> + if (res == 0) {
> + kfree(buf);
> + goto out;
> + }
> + /*
> + * Ensure NULL terminated but don't clobber the end
> + * unless the buffer is full. Worst case you end up
> + * with 2 null bytes ending it. By doing it this way
> + * one avoids additional branching. One checking if the
> + * end is null and another to check if their should be
> + * an increment before setting the null byte.
> + */
> + res += res < PATH_MAX;
> + buf[res-1] = '\0';
> context->cmdline = buf;
> }
> msg = context->cmdline;
> @@ -1333,8 +1344,8 @@ static void audit_log_exit(struct audit_context *context,
struct task_struct *ts
> context->name_count);
>
> audit_log_task_info(ab, tsk);
> - audit_log_cmdline(ab, tsk, context);
> audit_log_key(ab, context->filterkey);
> + audit_log_cmdline(ab, tsk, context);
> audit_log_end(ab);
>
> for (aux = context->aux; aux; aux = aux->next) {
3:37 p.m.
New subject: [PATCH] squash: review-updates
I think your right....
On Fri, Jan 10, 2014 at 1:08 PM, Eric Paris <eparis(a)redhat.com> wrote:
If you know the buf len, you can just use
audit_log_n_untrusted_string()
I think....
On Tue, 2014-01-07 at 12:44 -0800, William Roberts wrote:
> Signed-off-by: William Roberts <wroberts(a)tresys.com>
> ---
> kernel/auditsc.c | 19 +++++++++++++++----
> 1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index a4c2003..9ba1f2a 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1292,9 +1292,20 @@ static void audit_log_cmdline(struct audit_buffer *ab, struct
task_struct *tsk,
> if (!buf)
> goto out;
> res = get_cmdline(tsk, buf, PATH_MAX);
> - /* Ensure NULL terminated */
> - if (buf[res-1] != '\0')
> - buf[res-1] = '\0';
> + if (res == 0) {
> + kfree(buf);
> + goto out;
> + }
> + /*
> + * Ensure NULL terminated but don't clobber the end
> + * unless the buffer is full. Worst case you end up
> + * with 2 null bytes ending it. By doing it this way
> + * one avoids additional branching. One checking if the
> + * end is null and another to check if their should be
> + * an increment before setting the null byte.
> + */
> + res += res < PATH_MAX;
> + buf[res-1] = '\0';
> context->cmdline = buf;
> }
> msg = context->cmdline;
> @@ -1333,8 +1344,8 @@ static void audit_log_exit(struct audit_context *context,
struct task_struct *ts
> context->name_count);
>
> audit_log_task_info(ab, tsk);
> - audit_log_cmdline(ab, tsk, context);
> audit_log_key(ab, context->filterkey);
> + audit_log_cmdline(ab, tsk, context);
> audit_log_end(ab);
>
> for (aux = context->aux; aux; aux = aux->next) {
--
Respectfully,
William C Roberts
3:42 p.m.
New subject: [PATCH] squash: review-updates
hmm if I recall too the result from calling the underlying vma
functions was the width of the field, not
always necessarily the null byte, like you said, doing the combo you
suggest corrects that.
However, strnlen is pricey id like to avoid. The code I have at least
avoids branching, which is good.
And is then directly known to be safe to pass to
audit_log_untrusted_string(), which aborts printing
on the first null byte.
On Fri, Jan 10, 2014 at 1:37 PM, William Roberts
<bill.c.roberts(a)gmail.com> wrote:
I think your right....
On Fri, Jan 10, 2014 at 1:08 PM, Eric Paris <eparis(a)redhat.com> wrote:
> If you know the buf len, you can just use audit_log_n_untrusted_string()
> I think....
>
> On Tue, 2014-01-07 at 12:44 -0800, William Roberts wrote:
>> Signed-off-by: William Roberts <wroberts(a)tresys.com>
>> ---
>> kernel/auditsc.c | 19 +++++++++++++++----
>> 1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> index a4c2003..9ba1f2a 100644
>> --- a/kernel/auditsc.c
>> +++ b/kernel/auditsc.c
>> @@ -1292,9 +1292,20 @@ static void audit_log_cmdline(struct audit_buffer *ab,
struct task_struct *tsk,
>> if (!buf)
>> goto out;
>> res = get_cmdline(tsk, buf, PATH_MAX);
>> - /* Ensure NULL terminated */
>> - if (buf[res-1] != '\0')
>> - buf[res-1] = '\0';
>> + if (res == 0) {
>> + kfree(buf);
>> + goto out;
>> + }
>> + /*
>> + * Ensure NULL terminated but don't clobber the end
>> + * unless the buffer is full. Worst case you end up
>> + * with 2 null bytes ending it. By doing it this way
>> + * one avoids additional branching. One checking if the
>> + * end is null and another to check if their should be
>> + * an increment before setting the null byte.
>> + */
>> + res += res < PATH_MAX;
>> + buf[res-1] = '\0';
>> context->cmdline = buf;
>> }
>> msg = context->cmdline;
>> @@ -1333,8 +1344,8 @@ static void audit_log_exit(struct audit_context *context,
struct task_struct *ts
>> context->name_count);
>>
>> audit_log_task_info(ab, tsk);
>> - audit_log_cmdline(ab, tsk, context);
>> audit_log_key(ab, context->filterkey);
>> + audit_log_cmdline(ab, tsk, context);
>> audit_log_end(ab);
>>
>> for (aux = context->aux; aux; aux = aux->next) {
>
>
--
Respectfully,
William C Roberts
--
Respectfully,
William C Roberts
3998
days inactive
4001
days old
linux-audit@lists.linux-audit.osci.io
7 comments
3 participants
participants (3)
-
Eric Paris -
Richard Guy Briggs -
William Roberts