linux-kernel - Re: [PATCH 3/3] tomoyo: robustify handling of mm->exe

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <201502242035.GCI75431.LHQFOOJMFVSFtO@I-love.SAKURA.ne.jp>
Date:	Tue, 24 Feb 2015 20:35:26 +0900
From:	Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To:	dave@...olabs.net
Cc:	akpm@...ux-foundation.org, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org, takedakn@...data.co.jp,
	linux-security-module@...r.kernel.org
Subject: Re: [PATCH 3/3] tomoyo: robustify handling of mm->exe_file

Davidlohr Bueso wrote:
> On Fri, 2015-02-20 at 07:11 +0900, Tetsuo Handa wrote:
> > Davidlohr Bueso wrote:
> > > On Thu, 2015-02-19 at 20:07 +0900, Tetsuo Handa wrote:
> > > > Why do we need to let the caller call path_put() ?
> > > > There is no need to do like proc_exe_link() does, for
> > > > tomoyo_get_exe() returns pathname as "char *".
> > > 
> > > Having the pathname doesn't guarantee anything later, and thus doesn't
> > > seem very robust in the manager call if it can be dropped during the
> > > call... or can this never occur in this context?
> > > 
> > tomoyo_get_exe() returns the pathname of executable of current thread.
> > The executable of current thread cannot be changed while current thread
> > is inside the manager call. Although the pathname of executable of
> > current thread could be changed by other threads via namespace manipulation
> > like pivot_root(), holding a reference guarantees nothing. Your patch helps
> > for avoiding memory allocation with mmap_sem held, but does not robustify
> > handling of mm->exe_file for tomoyo.
> 
> Fair enough, I won't argue. This is beyond the scope if what I'm trying
> to accomplish here anyway. Are you ok with this instead?
> 
I prefer cleanups excluded from this patch, like shown below.
Would you please resend?

> diff --git a/security/tomoyo/common.c b/security/tomoyo/common.c
> index e0fb750..73ce629 100644
> --- a/security/tomoyo/common.c
> +++ b/security/tomoyo/common.c
> @@ -908,6 +908,31 @@ static void tomoyo_read_manager(struct tomoyo_io_buffer *head)
>  }
>  
>  /**
> + * tomoyo_get_exe - Get tomoyo_realpath() of current process.
> + *
> + * Returns the tomoyo_realpath() of current process on success, NULL otherwise.
> + *
> + * This function uses kzalloc(), so the caller must call kfree()
> + * if this function didn't return NULL.
> + */
> +static const char *tomoyo_get_exe(void)
> +{
> +	struct file *exe_file;
> +	const char *cp = NULL;

No need to initialize cp here.

> +	struct mm_struct *mm = current->mm;
> +
> +	if (!mm)
> +		return NULL;
> +	exe_file = get_mm_exe_file(mm);
> +	if (!exe_file)
> +		return NULL;
> +
> +	cp = tomoyo_realpath_from_path(&exe_file->f_path);
> +	fput(exe_file);
> +	return cp;
> +}
> +
> +/**
>   * tomoyo_manager - Check whether the current process is a policy manager.
>   *
>   * Returns true if the current process is permitted to modify policy
> diff --git a/security/tomoyo/common.h b/security/tomoyo/common.h
> index b897d48..fc89eba 100644
> --- a/security/tomoyo/common.h
> +++ b/security/tomoyo/common.h
> @@ -947,7 +947,6 @@ char *tomoyo_init_log(struct tomoyo_request_info *r, int len, const char *fmt,
>  char *tomoyo_read_token(struct tomoyo_acl_param *param);
>  char *tomoyo_realpath_from_path(struct path *path);
>  char *tomoyo_realpath_nofollow(const char *pathname);
> -const char *tomoyo_get_exe(void);
>  const char *tomoyo_yesno(const unsigned int value);
>  const struct tomoyo_path_info *tomoyo_compare_name_union
>  (const struct tomoyo_path_info *name, const struct tomoyo_name_union *ptr);
> diff --git a/security/tomoyo/util.c b/security/tomoyo/util.c
> index 2952ba5..7eff479 100644
> --- a/security/tomoyo/util.c
> +++ b/security/tomoyo/util.c
> @@ -939,28 +939,6 @@ bool tomoyo_path_matches_pattern(const struct tomoyo_path_info *filename,
>  }
>  
>  /**
> - * tomoyo_get_exe - Get tomoyo_realpath() of current process.
> - *
> - * Returns the tomoyo_realpath() of current process on success, NULL otherwise.
> - *
> - * This function uses kzalloc(), so the caller must call kfree()
> - * if this function didn't return NULL.
> - */
> -const char *tomoyo_get_exe(void)
> -{
> -	struct mm_struct *mm = current->mm;
> -	const char *cp = NULL;
> -
> -	if (!mm)
> -		return NULL;
> -	down_read(&mm->mmap_sem);
> -	if (mm->exe_file)
> -		cp = tomoyo_realpath_from_path(&mm->exe_file->f_path);
> -	up_read(&mm->mmap_sem);
> -	return cp;
> -}
> -
> -/**
>   * tomoyo_get_mode - Get MAC mode.
>   *
>   * @ns:      Pointer to "struct tomoyo_policy_namespace".
> -- 
> 2.1.4

You can post TOMOYO patches to tomoyo-dev-en@...ts.sourceforge.jp .
I'll add your mail address to ML's accept list.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/