linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Ingo Molnar <mingo@elte.hu>
To: Chris Wright <chrisw@sous-sol.org>
Cc: Oleg Nesterov <oleg@redhat.com>,
	Roland McGrath <roland@redhat.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	linux-kernel@vger.kernel.org, Al Viro <viro@ZenIV.linux.org.uk>
Subject: Re: [RFC PATCH 3/3a] ptrace: add _ptrace_may_access()
Date: Thu, 7 May 2009 11:04:59 +0200	[thread overview]
Message-ID: <20090507090459.GE19133@elte.hu> (raw)
In-Reply-To: <20090507085742.GB3036@sequoia.sous-sol.org>


* Chris Wright <chrisw@sous-sol.org> wrote:

> * Ingo Molnar (mingo@elte.hu) wrote:
> > * Oleg Nesterov <oleg@redhat.com> wrote:
> > > Agreed, but what about security_operations->ptrace_may_access ?
> > > It has the same (bad) name, but returns the error code or 0 on 
> > > success.
> > 
> > Bad code should generally be fixed, or in exceptional circumstances 
> > it can tolerated if it's pre-existing bad code, but it should never 
> > be propagated. It has not spread _that_ widely yet, and is isolated 
> > to the security subsystem:
> 
> And the security hooks tend to all follow the 0 success -ve ERR on error.

I just sent a patch (see below) that renames them to 
ptrace_access_check().

They have no active connection to the core kernel 
ptrace_may_access() check in any case: the methods seem to be 
home-brewn, security-module specific checks for how wide ptrace is 
allowed to go.

The design around that code does not seem to be very consistent.

One solution would be for the default "plain Linux" security module 
to have a stock ->ptrace_access_check() that does the current 
ptrace_may_access() check, and then procfs could be updated to use 
that callback - instead of calling into the ptrace core code 
directly.

This would mean that in the end the 'may' usage is eliminated 
altogether, and we have a clean ptrace_access_check() facility with 
a retval convention.

	Ingo

-------------->
Subject: security: rename ptrace_may_access => ptrace_access_check
From: Ingo Molnar <mingo@elte.hu>

The ->ptrace_may_access methods are named confusingly - the real
ptrace_may_access() returns a bool, while these security checks have
a retval convention.

Rename it to ptrace_access_check, to reduce the confusion factor.

[ Impact: cleanup, no code changed ]

Signed-off-by-if-you-test-it: Ingo Molnar <mingo@elte.hu>
---
 include/linux/security.h   |   14 +++++++-------
 security/capability.c      |    2 +-
 security/commoncap.c       |    4 ++--
 security/root_plug.c       |    2 +-
 security/security.c        |    4 ++--
 security/selinux/hooks.c   |    6 +++---
 security/smack/smack_lsm.c |    8 ++++----
 7 files changed, 20 insertions(+), 20 deletions(-)

Index: tip/include/linux/security.h
===================================================================
--- tip.orig/include/linux/security.h
+++ tip/include/linux/security.h
@@ -52,7 +52,7 @@ struct audit_krule;
 extern int cap_capable(struct task_struct *tsk, const struct cred *cred,
 		       int cap, int audit);
 extern int cap_settime(struct timespec *ts, struct timezone *tz);
-extern int cap_ptrace_may_access(struct task_struct *child, unsigned int mode);
+extern int cap_ptrace_access_check(struct task_struct *child, unsigned int mode);
 extern int cap_ptrace_traceme(struct task_struct *parent);
 extern int cap_capget(struct task_struct *target, kernel_cap_t *effective, kernel_cap_t *inheritable, kernel_cap_t *permitted);
 extern int cap_capset(struct cred *new, const struct cred *old,
@@ -1209,7 +1209,7 @@ static inline void security_free_mnt_opt
  *	@alter contains the flag indicating whether changes are to be made.
  *	Return 0 if permission is granted.
  *
- * @ptrace_may_access:
+ * @ptrace_access_check:
  *	Check permission before allowing the current process to trace the
  *	@child process.
  *	Security modules may also want to perform a process tracing check
@@ -1224,7 +1224,7 @@ static inline void security_free_mnt_opt
  *	Check that the @parent process has sufficient permission to trace the
  *	current process before allowing the current process to present itself
  *	to the @parent process for tracing.
- *	The parent process will still have to undergo the ptrace_may_access
+ *	The parent process will still have to undergo the ptrace_access_check
  *	checks before it is allowed to trace this one.
  *	@parent contains the task_struct structure for debugger process.
  *	Return 0 if permission is granted.
@@ -1336,7 +1336,7 @@ static inline void security_free_mnt_opt
 struct security_operations {
 	char name[SECURITY_NAME_MAX + 1];
 
-	int (*ptrace_may_access) (struct task_struct *child, unsigned int mode);
+	int (*ptrace_access_check) (struct task_struct *child, unsigned int mode);
 	int (*ptrace_traceme) (struct task_struct *parent);
 	int (*capget) (struct task_struct *target,
 		       kernel_cap_t *effective,
@@ -1617,7 +1617,7 @@ extern int security_module_enable(struct
 extern int register_security(struct security_operations *ops);
 
 /* Security operations */
-int security_ptrace_may_access(struct task_struct *child, unsigned int mode);
+int security_ptrace_access_check(struct task_struct *child, unsigned int mode);
 int security_ptrace_traceme(struct task_struct *parent);
 int security_capget(struct task_struct *target,
 		    kernel_cap_t *effective,
@@ -1798,10 +1798,10 @@ static inline int security_init(void)
 	return 0;
 }
 
-static inline int security_ptrace_may_access(struct task_struct *child,
+static inline int security_ptrace_access_check(struct task_struct *child,
 					     unsigned int mode)
 {
-	return cap_ptrace_may_access(child, mode);
+	return cap_ptrace_access_check(child, mode);
 }
 
 static inline int security_ptrace_traceme(struct task_struct *parent)
Index: tip/security/capability.c
===================================================================
--- tip.orig/security/capability.c
+++ tip/security/capability.c
@@ -867,7 +867,7 @@ struct security_operations default_secur
 
 void security_fixup_ops(struct security_operations *ops)
 {
-	set_to_cap_if_null(ops, ptrace_may_access);
+	set_to_cap_if_null(ops, ptrace_access_check);
 	set_to_cap_if_null(ops, ptrace_traceme);
 	set_to_cap_if_null(ops, capget);
 	set_to_cap_if_null(ops, capset);
Index: tip/security/commoncap.c
===================================================================
--- tip.orig/security/commoncap.c
+++ tip/security/commoncap.c
@@ -79,7 +79,7 @@ int cap_settime(struct timespec *ts, str
 }
 
 /**
- * cap_ptrace_may_access - Determine whether the current process may access
+ * cap_ptrace_access_check - Determine whether the current process may access
  *			   another
  * @child: The process to be accessed
  * @mode: The mode of attachment.
@@ -87,7 +87,7 @@ int cap_settime(struct timespec *ts, str
  * Determine whether a process may access another, returning 0 if permission
  * granted, -ve if denied.
  */
-int cap_ptrace_may_access(struct task_struct *child, unsigned int mode)
+int cap_ptrace_access_check(struct task_struct *child, unsigned int mode)
 {
 	int ret = 0;
 
Index: tip/security/root_plug.c
===================================================================
--- tip.orig/security/root_plug.c
+++ tip/security/root_plug.c
@@ -72,7 +72,7 @@ static int rootplug_bprm_check_security 
 
 static struct security_operations rootplug_security_ops = {
 	/* Use the capability functions for some of the hooks */
-	.ptrace_may_access =		cap_ptrace_may_access,
+	.ptrace_access_check =		cap_ptrace_access_check,
 	.ptrace_traceme =		cap_ptrace_traceme,
 	.capget =			cap_capget,
 	.capset =			cap_capset,
Index: tip/security/security.c
===================================================================
--- tip.orig/security/security.c
+++ tip/security/security.c
@@ -127,9 +127,9 @@ int register_security(struct security_op
 
 /* Security operations */
 
-int security_ptrace_may_access(struct task_struct *child, unsigned int mode)
+int security_ptrace_access_check(struct task_struct *child, unsigned int mode)
 {
-	return security_ops->ptrace_may_access(child, mode);
+	return security_ops->ptrace_access_check(child, mode);
 }
 
 int security_ptrace_traceme(struct task_struct *parent)
Index: tip/security/selinux/hooks.c
===================================================================
--- tip.orig/security/selinux/hooks.c
+++ tip/security/selinux/hooks.c
@@ -1854,12 +1854,12 @@ static inline u32 open_file_to_av(struct
 
 /* Hook functions begin here. */
 
-static int selinux_ptrace_may_access(struct task_struct *child,
+static int selinux_ptrace_access_check(struct task_struct *child,
 				     unsigned int mode)
 {
 	int rc;
 
-	rc = cap_ptrace_may_access(child, mode);
+	rc = cap_ptrace_access_check(child, mode);
 	if (rc)
 		return rc;
 
@@ -5318,7 +5318,7 @@ static int selinux_key_getsecurity(struc
 static struct security_operations selinux_ops = {
 	.name =				"selinux",
 
-	.ptrace_may_access =		selinux_ptrace_may_access,
+	.ptrace_access_check =		selinux_ptrace_access_check,
 	.ptrace_traceme =		selinux_ptrace_traceme,
 	.capget =			selinux_capget,
 	.capset =			selinux_capset,
Index: tip/security/smack/smack_lsm.c
===================================================================
--- tip.orig/security/smack/smack_lsm.c
+++ tip/security/smack/smack_lsm.c
@@ -92,7 +92,7 @@ struct inode_smack *new_inode_smack(char
  */
 
 /**
- * smack_ptrace_may_access - Smack approval on PTRACE_ATTACH
+ * smack_ptrace_access_check - Smack approval on PTRACE_ATTACH
  * @ctp: child task pointer
  * @mode: ptrace attachment mode
  *
@@ -100,11 +100,11 @@ struct inode_smack *new_inode_smack(char
  *
  * Do the capability checks, and require read and write.
  */
-static int smack_ptrace_may_access(struct task_struct *ctp, unsigned int mode)
+static int smack_ptrace_access_check(struct task_struct *ctp, unsigned int mode)
 {
 	int rc;
 
-	rc = cap_ptrace_may_access(ctp, mode);
+	rc = cap_ptrace_access_check(ctp, mode);
 	if (rc != 0)
 		return rc;
 
@@ -2826,7 +2826,7 @@ static void smack_release_secctx(char *s
 struct security_operations smack_ops = {
 	.name =				"smack",
 
-	.ptrace_may_access =		smack_ptrace_may_access,
+	.ptrace_access_check =		smack_ptrace_access_check,
 	.ptrace_traceme =		smack_ptrace_traceme,
 	.capget = 			cap_capget,
 	.capset = 			cap_capset,


  reply	other threads:[~2009-05-07  9:06 UTC|newest]

Thread overview: 51+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-05-05 22:47 [PATCH 3/3] ptrace: do not use task_lock() for attach Oleg Nesterov
2009-05-06  2:08 ` Roland McGrath
2009-05-06  8:00 ` [RFC PATCH 3/3a] ptrace: add _ptrace_may_access() Ingo Molnar
2009-05-06 20:32   ` Roland McGrath
2009-05-06 20:47     ` Christoph Hellwig
2009-05-06 21:09       ` Roland McGrath
2009-05-07  8:19       ` Ingo Molnar
2009-05-07  8:17     ` Ingo Molnar
2009-05-06 23:53   ` Oleg Nesterov
2009-05-07  0:21     ` Roland McGrath
2009-05-07  6:36       ` Oleg Nesterov
2009-05-07  8:20         ` Ingo Molnar
2009-05-07  8:31           ` Oleg Nesterov
2009-05-07  8:38             ` Ingo Molnar
2009-05-07  8:49               ` [patch] security: rename ptrace_may_access => ptrace_access_check Ingo Molnar
2009-05-07  9:19                 ` Oleg Nesterov
2009-05-07  9:27                   ` Ingo Molnar
2009-05-07  8:57               ` [RFC PATCH 3/3a] ptrace: add _ptrace_may_access() Chris Wright
2009-05-07  9:04                 ` Ingo Molnar [this message]
2009-05-07  9:20                   ` Chris Wright
2009-05-07  9:54                     ` James Morris
2009-05-07 10:20                       ` your mail Ingo Molnar
2009-05-07 11:37                         ` security: rename ptrace_may_access => ptrace_access_check James Morris
2009-05-07 14:17                           ` Ingo Molnar
2009-06-23 14:14                           ` Oleg Nesterov
2009-06-23 17:49                             ` Christoph Hellwig
2009-06-23 19:24                               ` [PATCH 0/1] mm_for_maps: simplify, use ptrace_may_access() Oleg Nesterov
2009-06-23 19:25                                 ` [PATCH 1/1] " Oleg Nesterov
2009-06-24  3:06                                   ` Serge E. Hallyn
2009-06-24 14:21                                     ` James Morris
2009-06-24  9:25                                   ` Roland McGrath
2009-06-24 14:37                                     ` Oleg Nesterov
2009-06-24  1:08                             ` security: rename ptrace_may_access => ptrace_access_check James Morris
2009-05-08  3:27                         ` your mail Casey Schaufler
2009-06-24 14:19                       ` security: rename ptrace_may_access => ptrace_access_check James Morris
2009-05-07  9:31                   ` [RFC PATCH 3/3a] ptrace: add _ptrace_may_access() Ingo Molnar
2009-05-07  9:49                     ` [patch 1/2] ptrace, security: rename ptrace_may_access => ptrace_access_check Ingo Molnar
2009-05-07 18:47                       ` Roland McGrath
2009-05-07 19:55                       ` Andrew Morton
2009-05-11 13:39                         ` Ingo Molnar
2009-05-11 18:51                           ` Andrew Morton
2009-05-15  1:10                           ` Américo Wang
2009-05-15 19:34                             ` Ingo Molnar
2009-05-07  9:50                     ` [patch 2/2] ptrace: turn ptrace_access_check() into a retval function Ingo Molnar
2009-05-07 18:47                       ` Roland McGrath
2009-05-06 22:46 ` [PATCH 3/3] ptrace: do not use task_lock() for attach Chris Wright
2009-05-06 23:13   ` Oleg Nesterov
2009-05-06 23:27     ` Chris Wright
2009-05-06 23:48       ` James Morris
2009-05-07  1:17         ` Roland McGrath
2009-05-08 12:18         ` David Howells

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20090507090459.GE19133@elte.hu \
    --to=mingo@elte.hu \
    --cc=akpm@linux-foundation.org \
    --cc=chrisw@sous-sol.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=oleg@redhat.com \
    --cc=roland@redhat.com \
    --cc=viro@ZenIV.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).