Re: [PATCH 1/4] LSM: Add security_bprm_aborting_creds() hook.

[ebiederm@xmission.com (Eric W. Biederman)]

Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes:

> Eric W. Biederman wrote:
>> Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes:
>> > diff --git a/fs/exec.c b/fs/exec.c
>> > index 8875dd1..89f0479 100644
>> > --- a/fs/exec.c
>> > +++ b/fs/exec.c
>> > @@ -1172,6 +1172,7 @@ void free_bprm(struct linux_binprm *bprm)
>> >  {
>> >  	free_arg_pages(bprm);
>> >  	if (bprm->cred) {
>> > +		security_bprm_aborting_creds(bprm);
>> 
>> Can you move this look outside of the cred_guard_mutex?  It looks like
>> you can and I expect not unnecessarily extending the scope of the mutex
>> would be a good idea.
>> 
>> >  		mutex_unlock(&current->signal->cred_guard_mutex);
>> >  		abort_creds(bprm->cred);
>> >  	}
>> 
>
> Sure. Here is the updated patch. Does this look OK to you?

Yes.  I don't know about the semantics of the patch but moving
oustide of the cred_guard_mutex looks good.

Eric



>>From b226aaf96303495a04d615cda6f8a06af3e822c3 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Fri, 18 Oct 2013 21:32:16 +0900
> Subject: [PATCHv2 1/4] LSM: Add security_bprm_aborting_creds() hook.
>
> Add a LSM hook which is called only when an execve operation failed after
> prepare_bprm_creds() succeeded. This hook is used by TOMOYO for synchronously
> cleaning up resources allocated during an execve operation.
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  fs/exec.c                |    1 +
>  include/linux/security.h |   11 +++++++++++
>  security/capability.c    |    5 +++++
>  security/security.c      |    5 +++++
>  4 files changed, 22 insertions(+), 0 deletions(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 8875dd1..b229f22 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1173,6 +1173,7 @@ void free_bprm(struct linux_binprm *bprm)
>  	free_arg_pages(bprm);
>  	if (bprm->cred) {
>  		mutex_unlock(&current->signal->cred_guard_mutex);
> +		security_bprm_aborting_creds(bprm);
>  		abort_creds(bprm->cred);
>  	}
>  	/* If a binfmt changed the interp, free it. */
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 9d37e2b..e9fff76 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -236,6 +236,11 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
>   *	linux_binprm structure.  This hook is a good place to perform state
>   *	changes on the process such as clearing out non-inheritable signal
>   *	state.  This is called immediately after commit_creds().
> + * @bprm_aborting_creds:
> + *	This hook is called when an execve operation failed after
> + *	prepare_bprm_creds() succeeded so that we can synchronously clean up
> + *	resources used by an execve operation.
> + *	@bprm points to the linux_binprm structure.
>   * @bprm_secureexec:
>   *	Return a boolean value (0 or 1) indicating whether a "secure exec"
>   *	is required.  The flag is passed in the auxiliary table
> @@ -1446,6 +1451,7 @@ struct security_operations {
>  	int (*bprm_secureexec) (struct linux_binprm *bprm);
>  	void (*bprm_committing_creds) (struct linux_binprm *bprm);
>  	void (*bprm_committed_creds) (struct linux_binprm *bprm);
> +	void (*bprm_aborting_creds) (struct linux_binprm *bprm);
>  
>  	int (*sb_alloc_security) (struct super_block *sb);
>  	void (*sb_free_security) (struct super_block *sb);
> @@ -1741,6 +1747,7 @@ int security_bprm_set_creds(struct linux_binprm *bprm);
>  int security_bprm_check(struct linux_binprm *bprm);
>  void security_bprm_committing_creds(struct linux_binprm *bprm);
>  void security_bprm_committed_creds(struct linux_binprm *bprm);
> +void security_bprm_aborting_creds(struct linux_binprm *bprm);
>  int security_bprm_secureexec(struct linux_binprm *bprm);
>  int security_sb_alloc(struct super_block *sb);
>  void security_sb_free(struct super_block *sb);
> @@ -1988,6 +1995,10 @@ static inline void security_bprm_committed_creds(struct linux_binprm *bprm)
>  {
>  }
>  
> +static inline void security_bprm_aborting_creds(struct linux_binprm *bprm)
> +{
> +}
> +
>  static inline int security_bprm_secureexec(struct linux_binprm *bprm)
>  {
>  	return cap_bprm_secureexec(bprm);
> diff --git a/security/capability.c b/security/capability.c
> index dbeb9bc..44ca607 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -40,6 +40,10 @@ static void cap_bprm_committed_creds(struct linux_binprm *bprm)
>  {
>  }
>  
> +static void cap_bprm_aborting_creds(struct linux_binprm *bprm)
> +{
> +}
> +
>  static int cap_sb_alloc_security(struct super_block *sb)
>  {
>  	return 0;
> @@ -931,6 +935,7 @@ void __init security_fixup_ops(struct security_operations *ops)
>  	set_to_cap_if_null(ops, bprm_set_creds);
>  	set_to_cap_if_null(ops, bprm_committing_creds);
>  	set_to_cap_if_null(ops, bprm_committed_creds);
> +	set_to_cap_if_null(ops, bprm_aborting_creds);
>  	set_to_cap_if_null(ops, bprm_check_security);
>  	set_to_cap_if_null(ops, bprm_secureexec);
>  	set_to_cap_if_null(ops, sb_alloc_security);
> diff --git a/security/security.c b/security/security.c
> index 4dc31f4..064da04 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -236,6 +236,11 @@ void security_bprm_committed_creds(struct linux_binprm *bprm)
>  	security_ops->bprm_committed_creds(bprm);
>  }
>  
> +void security_bprm_aborting_creds(struct linux_binprm *bprm)
> +{
> +	security_ops->bprm_aborting_creds(bprm);
> +}
> +
>  int security_bprm_secureexec(struct linux_binprm *bprm)
>  {
>  	return security_ops->bprm_secureexec(bprm);