linux-kernel - Re: [PATCH v6 6/7] seccomp: Introduce addfd ioctl to seccomp user notifier

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]

Message-ID: <20200709130811.zjyn6ptsd3rss3j4@wittgenstein>
Date:   Thu, 9 Jul 2020 15:08:11 +0200
From:   Christian Brauner <christian.brauner@...ntu.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     linux-kernel@...r.kernel.org, Sargun Dhillon <sargun@...gun.me>,
        Matt Denton <mpdenton@...gle.com>,
        Christian Brauner <christian@...uner.io>,
        Tycho Andersen <tycho@...ho.ws>,
        David Laight <David.Laight@...LAB.COM>,
        Christoph Hellwig <hch@....de>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Aleksa Sarai <cyphar@...har.com>, Jann Horn <jannh@...gle.com>,
        Chris Palmer <palmer@...gle.com>,
        Robert Sesek <rsesek@...gle.com>,
        Giuseppe Scrivano <gscrivan@...hat.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andy Lutomirski <luto@...capital.net>,
        Will Drewry <wad@...omium.org>, Shuah Khan <shuah@...nel.org>,
        netdev@...r.kernel.org, containers@...ts.linux-foundation.org,
        linux-api@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-kselftest@...r.kernel.org
Subject: Re: [PATCH v6 6/7] seccomp: Introduce addfd ioctl to seccomp user
 notifier

On Wed, Jul 08, 2020 at 11:12:02PM -0700, Kees Cook wrote:
> On Tue, Jul 07, 2020 at 03:30:49PM +0200, Christian Brauner wrote:
> > Hm, maybe change that description to sm like:
> > 
> > [...]
> 
> Cool, yeah. Thanks! I've tweaked it a little more
> 
> > > +	/* 24 is original sizeof(struct seccomp_notif_addfd) */
> > > +	if (size < 24 || size >= PAGE_SIZE)
> > > +		return -EINVAL;
> > 
> > Hm, so maybe add the following:
> > 
> > #define SECCOMP_NOTIFY_ADDFD_VER0 24
> > #define SECCOMP_NOTIFY_ADDFD_LATEST SECCOMP_NOTIFY_ADDFD_VER0
> > 
> > and then place:
> > 
> > BUILD_BUG_ON(sizeof(struct seccomp_notify_addfd) < SECCOMP_NOTIFY_ADDFD_VER0);
> > BUILD_BUG_ON(sizeof(struct open_how) != SECCOMP_NOTIFY_ADDFD_LATEST);
> 
> Yes, good idea (BTW, did the EA syscall docs land?)

I'll be giving a kernel summit talk about extensible syscalls to come to
some agreement on a few things. After this we'll update the doc patch
we have now and merge it. :)

> 
> I've made these SECCOMP_NOTIFY_ADDFD_SIZE_* to match your examples below
> (i.e.  I added "SIZE" to what you suggested above).

Yup, sounds good!

> 
> > somewhere which is what we do for clone3(), openat2() and others to
> > catch build-time nonsense.
> > 
> > include/uapi/linux/perf_event.h:#define PERF_ATTR_SIZE_VER0     64      /* sizeof first published struct */
> > include/uapi/linux/sched.h:#define CLONE_ARGS_SIZE_VER0 64 /* sizeof first published struct */
> > include/uapi/linux/sched/types.h:#define SCHED_ATTR_SIZE_VER0   48      /* sizeof first published struct */
> > include/linux/fcntl.h:#define OPEN_HOW_SIZE_VER0        24 /* sizeof first published struct */
> > include/linux/fcntl.h:#define OPEN_HOW_SIZE_LATEST      OPEN_HOW_SIZE_VER0
> 
> The ..._SIZE_VER0 and ...LATEST stuff doesn't seem useful to export via
> UAPI. Above, 2 of the 3 export to uapi. Is there a specific rationale
> for which should and which shouldn't?

I think openat2() just didn't think it was useful. I find them helpful
because I often update codebase to the newest struct I know about:

struct clone_args {
	__aligned_u64 flags;
	__aligned_u64 pidfd;
	__aligned_u64 child_tid;
	__aligned_u64 parent_tid;
	__aligned_u64 exit_signal;
	__aligned_u64 stack;
	__aligned_u64 stack_size;
	__aligned_u64 tls;
/* CLONE_ARGS_SIZE_VER0 64 */
	__aligned_u64 set_tid;
	__aligned_u64 set_tid_size;
/* CLONE_ARGS_SIZE_VER1 80 */
	__aligned_u64 cgroup;
/* CLONE_ARGS_SIZE_VER2 88 */
};

But bumping it means I can't use:

clone3(&clone_args, sizeof(clone));

everywhere in the codebase because I'm fscking over everyone on older
kernels now. :)

Soin various parts of the codebase I will just use:

clone3(&clone_args, CLONE_ARGS_SIZE_VER0);

because I don't care about any of the additional features and I don't
need the kernel to copy any of the other stuff. Then in other parts of
the codebase I want to set_tid so I use:

clone3(&clone_args, CLONE_ARGS_SIZE_VER1);

This way I can also set "templates", i.e.

struct clone_args clone_template1 = {
	.flags		|= CLONE_CLEAR_SIGHAND,
	.exit_signal	= SIGCHLD,
	.set_tid	= 1000,
	.set_tid_size	= 1,
};

and then use the same struct for:

clone3(&clone_template1, CLONE_ARGS_SIZE_VER0);
clone3(&clone_template1, CLONE_ARGS_SIZE_VER1);

Whereas sizeof(clone_template1) would always give me
CLONE_ARGS_SIZE_VER2.

Christian