[PATCH] lsm_audit: use get_task_comm

[PATCH] lsm_audit: use get_task_comm

[Geliang Tang]

get_task_comm() copys the task's comm under the task_lock, it's safer
than directly using memcpy().

Signed-off-by: Geliang Tang <geliangtang@gmail.com>
---
 security/lsm_audit.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/security/lsm_audit.c b/security/lsm_audit.c
index 28d4c3a..555b1c4 100644
--- a/security/lsm_audit.c
+++ b/security/lsm_audit.c
@@ -221,7 +221,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
 	BUILD_BUG_ON(sizeof(a->u) > sizeof(void *)*2);
 
 	audit_log_format(ab, " pid=%d comm=", task_tgid_nr(current));
-	audit_log_untrustedstring(ab, memcpy(comm, current->comm, sizeof(comm)));
+	audit_log_untrustedstring(ab, get_task_comm(comm, current));
 
 	switch (a->type) {
 	case LSM_AUDIT_DATA_NONE:
@@ -312,7 +312,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
 				char comm[sizeof(tsk->comm)];
 				audit_log_format(ab, " opid=%d ocomm=", pid);
 				audit_log_untrustedstring(ab,
-				    memcpy(comm, tsk->comm, sizeof(comm)));
+				    get_task_comm(comm, tsk));
 			}
 		}
 		break;
-- 
2.9.3

[PATCH] lsm_audit: use get_task_comm

[Geliang Tang]

get_task_comm() copys the task's comm under the task_lock, it's safer
than directly using memcpy().

Signed-off-by: Geliang Tang <geliangtang@gmail.com>
---
 security/lsm_audit.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/security/lsm_audit.c b/security/lsm_audit.c
index 28d4c3a..555b1c4 100644
--- a/security/lsm_audit.c
+++ b/security/lsm_audit.c
@@ -221,7 +221,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
 	BUILD_BUG_ON(sizeof(a->u) > sizeof(void *)*2);
 
 	audit_log_format(ab, " pid=%d comm=", task_tgid_nr(current));
-	audit_log_untrustedstring(ab, memcpy(comm, current->comm, sizeof(comm)));
+	audit_log_untrustedstring(ab, get_task_comm(comm, current));
 
 	switch (a->type) {
 	case LSM_AUDIT_DATA_NONE:
@@ -312,7 +312,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
 				char comm[sizeof(tsk->comm)];
 				audit_log_format(ab, " opid=%d ocomm=", pid);
 				audit_log_untrustedstring(ab,
-				    memcpy(comm, tsk->comm, sizeof(comm)));
+				    get_task_comm(comm, tsk));
 			}
 		}
 		break;
-- 
2.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] lsm_audit: use get_task_comm

[Paul Moore]

On Mon, Aug 28, 2017 at 9:58 AM, Geliang Tang <geliangtang@gmail.com> wrote:
> get_task_comm() copys the task's comm under the task_lock, it's safer
> than directly using memcpy().
>
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
>  security/lsm_audit.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/security/lsm_audit.c b/security/lsm_audit.c
> index 28d4c3a..555b1c4 100644
> --- a/security/lsm_audit.c
> +++ b/security/lsm_audit.c
> @@ -221,7 +221,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
>         BUILD_BUG_ON(sizeof(a->u) > sizeof(void *)*2);
>
>         audit_log_format(ab, " pid=%d comm=", task_tgid_nr(current));
> -       audit_log_untrustedstring(ab, memcpy(comm, current->comm, sizeof(comm)));
> +       audit_log_untrustedstring(ab, get_task_comm(comm, current));
>
>         switch (a->type) {
>         case LSM_AUDIT_DATA_NONE:
> @@ -312,7 +312,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
>                                 char comm[sizeof(tsk->comm)];
>                                 audit_log_format(ab, " opid=%d ocomm=", pid);
>                                 audit_log_untrustedstring(ab,
> -                                   memcpy(comm, tsk->comm, sizeof(comm)));
> +                                   get_task_comm(comm, tsk));

[NOTE: adding the linux-audit mailing list to this thread]

This isn't strictly a problem with this patch, but I think we should
be able to get rid of the 'comm' variable in this if-block as simply
reuse the 'comm' from the top of the function.  It would be nice to
include that in this patch.

Other than that minor nit, this patch looks good to me; if you make
that small change I'll merge it into the audit/next branch for the
upcoming merge window.

-- 
paul moore
www.paul-moore.com

[PATCH] lsm_audit: use get_task_comm

[Paul Moore]

On Mon, Aug 28, 2017 at 9:58 AM, Geliang Tang <geliangtang@gmail.com> wrote:
> get_task_comm() copys the task's comm under the task_lock, it's safer
> than directly using memcpy().
>
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
>  security/lsm_audit.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/security/lsm_audit.c b/security/lsm_audit.c
> index 28d4c3a..555b1c4 100644
> --- a/security/lsm_audit.c
> +++ b/security/lsm_audit.c
> @@ -221,7 +221,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
>         BUILD_BUG_ON(sizeof(a->u) > sizeof(void *)*2);
>
>         audit_log_format(ab, " pid=%d comm=", task_tgid_nr(current));
> -       audit_log_untrustedstring(ab, memcpy(comm, current->comm, sizeof(comm)));
> +       audit_log_untrustedstring(ab, get_task_comm(comm, current));
>
>         switch (a->type) {
>         case LSM_AUDIT_DATA_NONE:
> @@ -312,7 +312,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
>                                 char comm[sizeof(tsk->comm)];
>                                 audit_log_format(ab, " opid=%d ocomm=", pid);
>                                 audit_log_untrustedstring(ab,
> -                                   memcpy(comm, tsk->comm, sizeof(comm)));
> +                                   get_task_comm(comm, tsk));

[NOTE: adding the linux-audit mailing list to this thread]

This isn't strictly a problem with this patch, but I think we should
be able to get rid of the 'comm' variable in this if-block as simply
reuse the 'comm' from the top of the function.  It would be nice to
include that in this patch.

Other than that minor nit, this patch looks good to me; if you make
that small change I'll merge it into the audit/next branch for the
upcoming merge window.

-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] lsm_audit: use get_task_comm

[Richard Guy Briggs]

On 2017-08-28 17:54, Paul Moore wrote:
> On Mon, Aug 28, 2017 at 9:58 AM, Geliang Tang <geliangtang@gmail.com> wrote:
> > get_task_comm() copys the task's comm under the task_lock, it's safer
> > than directly using memcpy().
> >
> > Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> > ---
> >  security/lsm_audit.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/security/lsm_audit.c b/security/lsm_audit.c
> > index 28d4c3a..555b1c4 100644
> > --- a/security/lsm_audit.c
> > +++ b/security/lsm_audit.c
> > @@ -221,7 +221,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
> >         BUILD_BUG_ON(sizeof(a->u) > sizeof(void *)*2);
> >
> >         audit_log_format(ab, " pid=%d comm=", task_tgid_nr(current));
> > -       audit_log_untrustedstring(ab, memcpy(comm, current->comm, sizeof(comm)));
> > +       audit_log_untrustedstring(ab, get_task_comm(comm, current));
> >
> >         switch (a->type) {
> >         case LSM_AUDIT_DATA_NONE:
> > @@ -312,7 +312,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
> >                                 char comm[sizeof(tsk->comm)];
> >                                 audit_log_format(ab, " opid=%d ocomm=", pid);
> >                                 audit_log_untrustedstring(ab,
> > -                                   memcpy(comm, tsk->comm, sizeof(comm)));
> > +                                   get_task_comm(comm, tsk));
> 
> [NOTE: adding the linux-audit mailing list to this thread]

There was previously pushback about using get_task_comm() with its
locking, which is why in this particular location, a memcpy was chosen
instead.

This was done in:
5deeb5cece3f9b30c8129786726b9d02c412c8ca rgb 2015-04-14
("lsm: copy comm before calling audit_log to avoid race in string printing")

>From that commit:
    Using get_task_comm() to get a copy while acquiring the task_lock to prevent
    this and to prevent the result from being a mixture of old and new values of
    comm would incur potentially unacceptable overhead, considering that the value
    can be influenced by userspace and therefore untrusted anyways.

> This isn't strictly a problem with this patch, but I think we should
> be able to get rid of the 'comm' variable in this if-block as simply
> reuse the 'comm' from the top of the function.  It would be nice to
> include that in this patch.
> 
> Other than that minor nit, this patch looks good to me; if you make
> that small change I'll merge it into the audit/next branch for the
> upcoming merge window.

So, I'd offer a NACK here.

> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

[PATCH] lsm_audit: use get_task_comm

[Richard Guy Briggs]

On 2017-08-28 17:54, Paul Moore wrote:
> On Mon, Aug 28, 2017 at 9:58 AM, Geliang Tang <geliangtang@gmail.com> wrote:
> > get_task_comm() copys the task's comm under the task_lock, it's safer
> > than directly using memcpy().
> >
> > Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> > ---
> >  security/lsm_audit.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/security/lsm_audit.c b/security/lsm_audit.c
> > index 28d4c3a..555b1c4 100644
> > --- a/security/lsm_audit.c
> > +++ b/security/lsm_audit.c
> > @@ -221,7 +221,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
> >         BUILD_BUG_ON(sizeof(a->u) > sizeof(void *)*2);
> >
> >         audit_log_format(ab, " pid=%d comm=", task_tgid_nr(current));
> > -       audit_log_untrustedstring(ab, memcpy(comm, current->comm, sizeof(comm)));
> > +       audit_log_untrustedstring(ab, get_task_comm(comm, current));
> >
> >         switch (a->type) {
> >         case LSM_AUDIT_DATA_NONE:
> > @@ -312,7 +312,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
> >                                 char comm[sizeof(tsk->comm)];
> >                                 audit_log_format(ab, " opid=%d ocomm=", pid);
> >                                 audit_log_untrustedstring(ab,
> > -                                   memcpy(comm, tsk->comm, sizeof(comm)));
> > +                                   get_task_comm(comm, tsk));
> 
> [NOTE: adding the linux-audit mailing list to this thread]

There was previously pushback about using get_task_comm() with its
locking, which is why in this particular location, a memcpy was chosen
instead.

This was done in:
5deeb5cece3f9b30c8129786726b9d02c412c8ca rgb 2015-04-14
("lsm: copy comm before calling audit_log to avoid race in string printing")

>From that commit:
    Using get_task_comm() to get a copy while acquiring the task_lock to prevent
    this and to prevent the result from being a mixture of old and new values of
    comm would incur potentially unacceptable overhead, considering that the value
    can be influenced by userspace and therefore untrusted anyways.

> This isn't strictly a problem with this patch, but I think we should
> be able to get rid of the 'comm' variable in this if-block as simply
> reuse the 'comm' from the top of the function.  It would be nice to
> include that in this patch.
> 
> Other than that minor nit, this patch looks good to me; if you make
> that small change I'll merge it into the audit/next branch for the
> upcoming merge window.

So, I'd offer a NACK here.

> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] lsm_audit: use get_task_comm

[Richard Guy Briggs]

On 2017-08-28 17:54, Paul Moore wrote:
> On Mon, Aug 28, 2017 at 9:58 AM, Geliang Tang <geliangtang@gmail.com> wrote:
> > get_task_comm() copys the task's comm under the task_lock, it's safer
> > than directly using memcpy().
> >
> > Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> > ---
> >  security/lsm_audit.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/security/lsm_audit.c b/security/lsm_audit.c
> > index 28d4c3a..555b1c4 100644
> > --- a/security/lsm_audit.c
> > +++ b/security/lsm_audit.c
> > @@ -221,7 +221,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
> >         BUILD_BUG_ON(sizeof(a->u) > sizeof(void *)*2);
> >
> >         audit_log_format(ab, " pid=%d comm=", task_tgid_nr(current));
> > -       audit_log_untrustedstring(ab, memcpy(comm, current->comm, sizeof(comm)));
> > +       audit_log_untrustedstring(ab, get_task_comm(comm, current));
> >
> >         switch (a->type) {
> >         case LSM_AUDIT_DATA_NONE:
> > @@ -312,7 +312,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
> >                                 char comm[sizeof(tsk->comm)];
> >                                 audit_log_format(ab, " opid=%d ocomm=", pid);
> >                                 audit_log_untrustedstring(ab,
> > -                                   memcpy(comm, tsk->comm, sizeof(comm)));
> > +                                   get_task_comm(comm, tsk));
> 
> [NOTE: adding the linux-audit mailing list to this thread]

There was previously pushback about using get_task_comm() with its
locking, which is why in this particular location, a memcpy was chosen
instead.

This was done in:
5deeb5cece3f9b30c8129786726b9d02c412c8ca rgb 2015-04-14
("lsm: copy comm before calling audit_log to avoid race in string printing")

>From that commit:
    Using get_task_comm() to get a copy while acquiring the task_lock to prevent
    this and to prevent the result from being a mixture of old and new values of
    comm would incur potentially unacceptable overhead, considering that the value
    can be influenced by userspace and therefore untrusted anyways.

> This isn't strictly a problem with this patch, but I think we should
> be able to get rid of the 'comm' variable in this if-block as simply
> reuse the 'comm' from the top of the function.  It would be nice to
> include that in this patch.
> 
> Other than that minor nit, this patch looks good to me; if you make
> that small change I'll merge it into the audit/next branch for the
> upcoming merge window.

So, I'd offer a NACK here.

> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [PATCH] lsm_audit: use get_task_comm

[Paul Moore]

On Tue, Aug 29, 2017 at 1:06 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2017-08-28 17:54, Paul Moore wrote:
>> On Mon, Aug 28, 2017 at 9:58 AM, Geliang Tang <geliangtang@gmail.com> wrote:
>> > get_task_comm() copys the task's comm under the task_lock, it's safer
>> > than directly using memcpy().
>> >
>> > Signed-off-by: Geliang Tang <geliangtang@gmail.com>
>> > ---
>> >  security/lsm_audit.c | 4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/security/lsm_audit.c b/security/lsm_audit.c
>> > index 28d4c3a..555b1c4 100644
>> > --- a/security/lsm_audit.c
>> > +++ b/security/lsm_audit.c
>> > @@ -221,7 +221,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
>> >         BUILD_BUG_ON(sizeof(a->u) > sizeof(void *)*2);
>> >
>> >         audit_log_format(ab, " pid=%d comm=", task_tgid_nr(current));
>> > -       audit_log_untrustedstring(ab, memcpy(comm, current->comm, sizeof(comm)));
>> > +       audit_log_untrustedstring(ab, get_task_comm(comm, current));
>> >
>> >         switch (a->type) {
>> >         case LSM_AUDIT_DATA_NONE:
>> > @@ -312,7 +312,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
>> >                                 char comm[sizeof(tsk->comm)];
>> >                                 audit_log_format(ab, " opid=%d ocomm=", pid);
>> >                                 audit_log_untrustedstring(ab,
>> > -                                   memcpy(comm, tsk->comm, sizeof(comm)));
>> > +                                   get_task_comm(comm, tsk));
>>
>> [NOTE: adding the linux-audit mailing list to this thread]
>
> There was previously pushback about using get_task_comm() with its
> locking, which is why in this particular location, a memcpy was chosen
> instead.
>
> This was done in:
> 5deeb5cece3f9b30c8129786726b9d02c412c8ca rgb 2015-04-14
> ("lsm: copy comm before calling audit_log to avoid race in string printing")
>
> From that commit:
>     Using get_task_comm() to get a copy while acquiring the task_lock to prevent
>     this and to prevent the result from being a mixture of old and new values of
>     comm would incur potentially unacceptable overhead, considering that the value
>     can be influenced by userspace and therefore untrusted anyways.
>

Ah ha!  Thanks for that, I had a hunch this had come up before and the
locking was an issue, I just couldn't find it while searching quickly
yesterday.

So yes, NACK to this patch.

-- 
paul moore
www.paul-moore.com

[PATCH] lsm_audit: use get_task_comm

[Paul Moore]

On Tue, Aug 29, 2017 at 1:06 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2017-08-28 17:54, Paul Moore wrote:
>> On Mon, Aug 28, 2017 at 9:58 AM, Geliang Tang <geliangtang@gmail.com> wrote:
>> > get_task_comm() copys the task's comm under the task_lock, it's safer
>> > than directly using memcpy().
>> >
>> > Signed-off-by: Geliang Tang <geliangtang@gmail.com>
>> > ---
>> >  security/lsm_audit.c | 4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/security/lsm_audit.c b/security/lsm_audit.c
>> > index 28d4c3a..555b1c4 100644
>> > --- a/security/lsm_audit.c
>> > +++ b/security/lsm_audit.c
>> > @@ -221,7 +221,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
>> >         BUILD_BUG_ON(sizeof(a->u) > sizeof(void *)*2);
>> >
>> >         audit_log_format(ab, " pid=%d comm=", task_tgid_nr(current));
>> > -       audit_log_untrustedstring(ab, memcpy(comm, current->comm, sizeof(comm)));
>> > +       audit_log_untrustedstring(ab, get_task_comm(comm, current));
>> >
>> >         switch (a->type) {
>> >         case LSM_AUDIT_DATA_NONE:
>> > @@ -312,7 +312,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
>> >                                 char comm[sizeof(tsk->comm)];
>> >                                 audit_log_format(ab, " opid=%d ocomm=", pid);
>> >                                 audit_log_untrustedstring(ab,
>> > -                                   memcpy(comm, tsk->comm, sizeof(comm)));
>> > +                                   get_task_comm(comm, tsk));
>>
>> [NOTE: adding the linux-audit mailing list to this thread]
>
> There was previously pushback about using get_task_comm() with its
> locking, which is why in this particular location, a memcpy was chosen
> instead.
>
> This was done in:
> 5deeb5cece3f9b30c8129786726b9d02c412c8ca rgb 2015-04-14
> ("lsm: copy comm before calling audit_log to avoid race in string printing")
>
> From that commit:
>     Using get_task_comm() to get a copy while acquiring the task_lock to prevent
>     this and to prevent the result from being a mixture of old and new values of
>     comm would incur potentially unacceptable overhead, considering that the value
>     can be influenced by userspace and therefore untrusted anyways.
>

Ah ha!  Thanks for that, I had a hunch this had come up before and the
locking was an issue, I just couldn't find it while searching quickly
yesterday.

So yes, NACK to this patch.

-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html