On Thu, 25 Feb 2010, Tetsuo Handa wrote:
> James Morris wrote:
> > On Wed, 24 Feb 2010, Tetsuo Handa wrote:
> >
> > > Caller of find_task_by_vpid() must use rcu_read_lock().
> >
> > The comment for the function says:
> >
> > Must be called under rcu_read_lock() or with tasklist_lock read-held.
> >
> > In the cases below, you have taken tasklist_lock.
>
> That comment will be updated to
> "Must be called under rcu_read_lock()." in 2.6.34 .
> Please see http://lkml.org/lkml/2009/12/10/180 and http://lkml.org/lkml/2009/12/9/472 .
Ok, please fix the comment as well.
Thomas: wasn't Tetsuo correct that we need both the rcu lock and
tasklist_lock held to call this?
Also, I think the audit code needs fixing.
--
James Morris
<[email protected]>
James Morris wrote:
> On Thu, 25 Feb 2010, Tetsuo Handa wrote:
>
> > James Morris wrote:
> > > On Wed, 24 Feb 2010, Tetsuo Handa wrote:
> > >
> > > > Caller of find_task_by_vpid() must use rcu_read_lock().
> > >
> > > The comment for the function says:
> > >
> > > Must be called under rcu_read_lock() or with tasklist_lock read-held.
> > >
> > > In the cases below, you have taken tasklist_lock.
> >
> > That comment will be updated to
> > "Must be called under rcu_read_lock()." in 2.6.34 .
The patch for updating that comment is at
http://userweb.kernel.org/~akpm/mmotm/broken-out/kernel-pidc-update-comment-on-find_task_by_pid_ns.patch
> > Please see http://lkml.org/lkml/2009/12/10/180 and http://lkml.org/lkml/2009/12/9/472 .
>
> Ok, please fix the comment as well.
I see. Explanation is in commit d4581a239a40319205762b76c01eb6363f277efa
"sys: Fix missing rcu protection for __task_cred() access".
Thus, I think below comment should be sufficient.
--------------------
[PATCH] TOMOYO: Protect find_task_by_vpid() with RCU.
Holding tasklist_lock is no longer sufficient for find_task_by_vpid().
Explicit rcu_read_lock() is required.
Signed-off-by: Tetsuo Handa <[email protected]>
--
security/tomoyo/common.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/security/tomoyo/common.c b/security/tomoyo/common.c
index 30dd1c6..ff51f10 100644
--- a/security/tomoyo/common.c
+++ b/security/tomoyo/common.c
@@ -1232,11 +1232,13 @@ static bool tomoyo_is_select_one(struct tomoyo_io_buffer *head,
if (sscanf(data, "pid=%u", &pid) == 1) {
struct task_struct *p;
+ rcu_read_lock();
read_lock(&tasklist_lock);
p = find_task_by_vpid(pid);
if (p)
domain = tomoyo_real_domain(p);
read_unlock(&tasklist_lock);
+ rcu_read_unlock();
} else if (!strncmp(data, "domain=", 7)) {
if (tomoyo_is_domain_def(data + 7))
domain = tomoyo_find_domain(data + 7);
@@ -1635,11 +1637,13 @@ static int tomoyo_read_pid(struct tomoyo_io_buffer *head)
const int pid = head->read_step;
struct task_struct *p;
struct tomoyo_domain_info *domain = NULL;
+ rcu_read_lock();
read_lock(&tasklist_lock);
p = find_task_by_vpid(pid);
if (p)
domain = tomoyo_real_domain(p);
read_unlock(&tasklist_lock);
+ rcu_read_unlock();
if (domain)
tomoyo_io_printf(head, "%d %u %s", pid, domain->profile,
domain->domainname->name);
On Thu, 25 Feb 2010, Tetsuo Handa wrote:
> I see. Explanation is in commit d4581a239a40319205762b76c01eb6363f277efa
> "sys: Fix missing rcu protection for __task_cred() access".
> Thus, I think below comment should be sufficient.
> --------------------
> [PATCH] TOMOYO: Protect find_task_by_vpid() with RCU.
>
> Holding tasklist_lock is no longer sufficient for find_task_by_vpid().
> Explicit rcu_read_lock() is required.
Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6#next
The audit code still looks like it needs to be updated.
--
James Morris
<[email protected]>