linux-kernel - Re: + proc-export-a-processes-resource-limits-via-proc-pid.patch added to -mm tree

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: <20070818115816.GA23555@hmsreliant.think-freely.org>
Date:	Sat, 18 Aug 2007 07:58:16 -0400
From:	Neil Horman <nhorman@...driver.com>
To:	Oleg Nesterov <oleg@...sign.ru>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: + proc-export-a-processes-resource-limits-via-proc-pid.patch added to -mm tree

On Sat, Aug 18, 2007 at 02:22:28AM +0400, Oleg Nesterov wrote:
> Neil Horman wrote:
> > 
> > +static int proc_pid_limits(struct task_struct *task, char *buffer)
> > +{
> > +	unsigned int i;
> > +	int count = 0;
> > +	char *bufptr = buffer;
> > +
> > +	struct rlimit rlim[RLIM_NLIMITS];
> > +
> > +	read_lock(&tasklist_lock);
> > +	memcpy(rlim, task->signal->rlim, sizeof(struct rlimit) * RLIM_NLIMITS);
> > +	read_unlock(&tasklist_lock);
> 
> Please don't re-introduce tasklist_lock unless strictly needed. And in this case
> it doesn't help, sys_getrlimit() changes ->rlim[] under task_lock().
> 
> Hovewer, I think the whole patch is not right. The "tsk" itself is pinned, but its
> ->signal is not stable and can be == NULL.
> 
> You can use lock_task_sighand() to access ->signal.
> 
You're right about the use of task_lock rather than tasklist_lock in getrlimit,
but the comment from lock_task_sighand indicates that its use is predicated on
the prerequisite of locking tasklist_lock, so I think the situation is not that
much of an issue.  From what I see the use of lock_task_sighand is used when
modifying values in the signal struct, not when removing it entirely (IIRC it
needs to exist until such time as all sharing processes exit.  The fact that we
have an outstanding task struct we are using here guarantees its continued
existence).  Since we are only reading signal->rlimit, which is only written to
from sys_setrlimit, we should be safe from corrupted limit reads, which at worst
would cause an erroneous transient data read, rather than any sort of
panic/crash.

Regards 
Neil

> Oleg.

-- 
/***************************************************
 *Neil Horman
 *Software Engineer
 *Red Hat, Inc.
 *nhorman@...driver.com
 *gpg keyid: 1024D / 0x92A74FA1
 *http://pgp.mit.edu
 ***************************************************/
-
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/