[PATCH] Yama: access current->comm directly

[PATCH] Yama: access current->comm directly

[Kees Cook]

The core ptrace access checking routine already holds the task lock,
so there is no need to use get_task_comm() which just tries to take the
lock again. Drop its use and access current->comm directly.

Reported-by: Fengguang Wu <fengguang.wu@intel.com>
Suggested-by: Oleg Nesterov <oleg@redhat.com>
Cc: stable@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 security/yama/yama_lsm.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
index 83554ee..20a68ca 100644
--- a/security/yama/yama_lsm.c
+++ b/security/yama/yama_lsm.c
@@ -279,12 +279,10 @@ static int yama_ptrace_access_check(struct task_struct *child,
 	}
 
 	if (rc) {
-		char name[sizeof(current->comm)];
+		/* task_lock is held by the caller, so use comm directly. */
 		printk_ratelimited(KERN_NOTICE
 			"ptrace of pid %d was attempted by: %s (pid %d)\n",
-			child->pid,
-			get_task_comm(name, current),
-			current->pid);
+			child->pid, current->comm, current->pid);
 	}
 
 	return rc;
-- 
1.7.0.4


-- 
Kees Cook
Chrome OS Security

Re: [PATCH] Yama: access current->comm directly

[John Johansen]

On 07/30/2012 09:58 AM, Kees Cook wrote:
> The core ptrace access checking routine already holds the task lock,
> so there is no need to use get_task_comm() which just tries to take the
> lock again. Drop its use and access current->comm directly.
> 
> Reported-by: Fengguang Wu <fengguang.wu@intel.com>
> Suggested-by: Oleg Nesterov <oleg@redhat.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
yep, looks good

Acked-by: John Johansen <john.johansen@canonical.com>

> ---
>  security/yama/yama_lsm.c |    6 ++----
>  1 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
> index 83554ee..20a68ca 100644
> --- a/security/yama/yama_lsm.c
> +++ b/security/yama/yama_lsm.c
> @@ -279,12 +279,10 @@ static int yama_ptrace_access_check(struct task_struct *child,
>  	}
>  
>  	if (rc) {
> -		char name[sizeof(current->comm)];
> +		/* task_lock is held by the caller, so use comm directly. */
>  		printk_ratelimited(KERN_NOTICE
>  			"ptrace of pid %d was attempted by: %s (pid %d)\n",
> -			child->pid,
> -			get_task_comm(name, current),
> -			current->pid);
> +			child->pid, current->comm, current->pid);
>  	}
>  
>  	return rc;
> 

Re: [PATCH] Yama: access current->comm directly

[Alan Cox]

On Mon, 30 Jul 2012 14:21:33 -0700
John Johansen <john.johansen@canonical.com> wrote:

> On 07/30/2012 09:58 AM, Kees Cook wrote:
> > The core ptrace access checking routine already holds the task lock,
> > so there is no need to use get_task_comm() which just tries to take the
> > lock again. Drop its use and access current->comm directly.
> > 
> > Reported-by: Fengguang Wu <fengguang.wu@intel.com>
> > Suggested-by: Oleg Nesterov <oleg@redhat.com>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> yep, looks good
> 
> Acked-by: John Johansen <john.johansen@canonical.com>


Code looks correct but having a weird cornercase direct reference is
asking for an accident later. For maintainability I'd rather see that as
__get_task_comm() or get_task_commu_unlocked() which does the job, deals
with all the edge cases and contains a WARN_ON check on the lock.

Alan

Re: [PATCH] Yama: access current->comm directly

[Kees Cook]

Hi,

On Mon, Jul 30, 2012 at 2:50 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> On Mon, 30 Jul 2012 14:21:33 -0700
> John Johansen <john.johansen@canonical.com> wrote:
>
>> On 07/30/2012 09:58 AM, Kees Cook wrote:
>> > The core ptrace access checking routine already holds the task lock,
>> > so there is no need to use get_task_comm() which just tries to take the
>> > lock again. Drop its use and access current->comm directly.
>> >
>> > Reported-by: Fengguang Wu <fengguang.wu@intel.com>
>> > Suggested-by: Oleg Nesterov <oleg@redhat.com>
>> > Cc: stable@vger.kernel.org
>> > Signed-off-by: Kees Cook <keescook@chromium.org>
>> yep, looks good
>>
>> Acked-by: John Johansen <john.johansen@canonical.com>
>
>
> Code looks correct but having a weird cornercase direct reference is
> asking for an accident later. For maintainability I'd rather see that as
> __get_task_comm() or get_task_commu_unlocked() which does the job, deals
> with all the edge cases and contains a WARN_ON check on the lock.

I'd be happy to refactor this area, sure. What's the best way to check
for this lock. I see in staging:

#define ASSERT_SPINLOCK(_l)     WARN_ON(!spin_is_locked(_l))

but in other areas, I see things using the raw_ prefix:

                WARN_ON(raw_spin_is_locked(&kvm_lock));

Which is preferred for task_lock?

-Kees

-- 
Kees Cook
Chrome OS Security