linux-kernel - Re: [PATCH 5/6] core: allow setrlimit to non-current tasks

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] [day] [month] [year] [list]

Date:	Tue, 1 Sep 2009 18:22:15 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Jiri Slaby <jirislaby@...il.com>
Cc:	akpm@...ux-foundation.org, mingo@...hat.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/6] core: allow setrlimit to non-current tasks

Sorry, can't read these series today. Will try tomorrow.

But at first glance some parts looks suspicious to me,

On 08/31, Jiri Slaby wrote:
>
> @@ -1244,16 +1244,27 @@ int setrlimit(struct task_struct *tsk, unsigned int resource,
>
>  	if (new_rlim->rlim_cur > new_rlim->rlim_max)
>  		return -EINVAL;
> +
> +	/* protect tsk->signal and tsk->sighand from disappearing */
> +	read_lock_irq(&tasklist_lock);

Why _irq? We can take tasklist_lock for reading without disabling irqs.

And. Unless I misread the patch, update_rlimit_cpu() is called before
read_unlock_irq(&tasklist_lock), but update_rlimit_cpu() does
spin_unlock_irq(->siglock) and restores interrupts.

> +	if (!tsk->signal || !tsk->sighand) {

Please don't check !tsk->signal, !tsk->sighand is enough. If
we have ->sighand != NULL (under lock) ->signal must be valid.

But I dislike the fact the patch uses tasklist_lock. Can't
lock_task_sighand() work for you? (of course, in this case
update_rlimit_cpu() should be updated too).

Once again, I didn't actually read this series yet, sorry.

Oleg.

--
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/