linux-kernel - Re: [PATCH v3] fs/proc: Expose RSEQ configuration

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]

Date:   Tue, 26 Jan 2021 15:58:46 -0500 (EST)
From:   Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:     Piotr Figiel <figiel@...gle.com>
Cc:     Alexey Dobriyan <adobriyan@...il.com>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Kees Cook <keescook@...omium.org>,
        Alexey Gladkov <gladkov.alexey@...il.com>,
        Michel Lespinasse <walken@...gle.com>,
        Bernd Edlinger <bernd.edlinger@...mail.de>,
        Andrei Vagin <avagin@...il.com>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Peter Zijlstra <peterz@...radead.org>,
        paulmck <paulmck@...nel.org>, Boqun Feng <boqun.feng@...il.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Peter Oskolkov <posk@...gle.com>,
        Kamil Yurtsever <kyurtsever@...gle.com>,
        Chris Kennelly <ckennelly@...gle.com>,
        Paul Turner <pjt@...gle.com>
Subject: Re: [PATCH v3] fs/proc: Expose RSEQ configuration

----- On Jan 26, 2021, at 1:54 PM, Piotr Figiel figiel@...gle.com wrote:
[...]
> diff --git a/kernel/rseq.c b/kernel/rseq.c
> index a4f86a9d6937..6aea67878065 100644
> --- a/kernel/rseq.c
> +++ b/kernel/rseq.c
> @@ -322,8 +322,10 @@ SYSCALL_DEFINE4(rseq, struct rseq __user *, rseq, u32,
> rseq_len,
> 		ret = rseq_reset_rseq_cpu_id(current);
> 		if (ret)
> 			return ret;
> +		task_lock(current);
> 		current->rseq = NULL;
> 		current->rseq_sig = 0;
> +		task_unlock(current);
> 		return 0;
> 	}
> 
> @@ -353,8 +355,10 @@ SYSCALL_DEFINE4(rseq, struct rseq __user *, rseq, u32,
> rseq_len,
> 		return -EINVAL;
> 	if (!access_ok(rseq, rseq_len))
> 		return -EFAULT;
> +	task_lock(current);
> 	current->rseq = rseq;
> 	current->rseq_sig = sig;
> +	task_unlock(current);

So AFAIU, the locks are there to make sure that whenever a user-space thread reads
that state through that new /proc file ABI, it observes coherent "rseq" vs "rseq_sig"
values. However, I'm not convinced this is the right approach to consistency here.

Because if you add locking as done here, you ensure that the /proc file reader
sees coherent values, but between the point where those values are read from
kernel-space, copied to user-space, and then acted upon by user-space, those can
very well have become outdated if the observed process runs concurrently.

So my understanding here is that the only non-racy way to effectively use those
values is to either read them from /proc/self/* (from the thread owning the task struct),
or to ensure that the thread is stopped/frozen while the read is done.

Maybe we should consider validating that the proc file is used from the right context
(from self or when the target thread is stopped/frozen) rather than add dubious locking ?

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com