Linux-api Archive on lore.kernel.org
 help / color / Atom feed
From: "Mickaël Salaün" <mic@digikod.net>
To: Al Viro <viro@zeniv.linux.org.uk>
Cc: "Jann Horn" <jannh@google.com>,
	"Kees Cook" <keescook@chromium.org>,
	"Andrew Morton" <akpm@linux-foundation.org>,
	"Andy Lutomirski" <luto@amacapital.net>,
	"Anton Ivanov" <anton.ivanov@cambridgegreys.com>,
	"Arnd Bergmann" <arnd@arndb.de>,
	"Casey Schaufler" <casey@schaufler-ca.com>,
	"David Howells" <dhowells@redhat.com>,
	"Jeff Dike" <jdike@addtoit.com>,
	"Jonathan Corbet" <corbet@lwn.net>,
	"Michael Kerrisk" <mtk.manpages@gmail.com>,
	"Richard Weinberger" <richard@nod.at>,
	"Shuah Khan" <shuah@kernel.org>,
	"Vincent Dagonneau" <vincent.dagonneau@ssi.gouv.fr>,
	kernel-hardening@lists.openwall.com, linux-api@vger.kernel.org,
	linux-arch@vger.kernel.org, linux-doc@vger.kernel.org,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-kselftest@vger.kernel.org,
	linux-security-module@vger.kernel.org, x86@kernel.org,
	"Mickaël Salaün" <mic@linux.microsoft.com>,
	"James Morris" <jmorris@namei.org>,
	"Serge E . Hallyn" <serge@hallyn.com>
Subject: Re: [PATCH v31 07/12] landlock: Support filesystem access-control
Date: Thu, 1 Apr 2021 19:12:05 +0200
Message-ID: <84e1cd29-0f09-1ed4-c680-65ca8c6988a3@digikod.net> (raw)
In-Reply-To: <YGUslUPwp85Zrp4t@zeniv-ca.linux.org.uk>


On 01/04/2021 04:14, Al Viro wrote:
> On Wed, Mar 31, 2021 at 07:33:50PM +0200, Mickaël Salaün wrote:
> 
>>> +static inline u64 unmask_layers(
>>> +		const struct landlock_ruleset *const domain,
>>> +		const struct path *const path, const u32 access_request,
>>> +		u64 layer_mask)
>>> +{
>>> +	const struct landlock_rule *rule;
>>> +	const struct inode *inode;
>>> +	size_t i;
>>> +
>>> +	if (d_is_negative(path->dentry))
>>> +		/* Continues to walk while there is no mapped inode. */
> 				     ^^^^^
> Odd comment, that...

I'll replace that with something more appropriate, e.g. "Ignore
nonexistent leafs".

> 
>>> +static int check_access_path(const struct landlock_ruleset *const domain,
>>> +		const struct path *const path, u32 access_request)
>>> +{
> 
>>> +	walker_path = *path;
>>> +	path_get(&walker_path);
> 
>>> +	while (true) {
>>> +		struct dentry *parent_dentry;
>>> +
>>> +		layer_mask = unmask_layers(domain, &walker_path,
>>> +				access_request, layer_mask);
>>> +		if (layer_mask == 0) {
>>> +			/* Stops when a rule from each layer grants access. */
>>> +			allowed = true;
>>> +			break;
>>> +		}
>>> +
>>> +jump_up:
>>> +		if (walker_path.dentry == walker_path.mnt->mnt_root) {
>>> +			if (follow_up(&walker_path)) {
>>> +				/* Ignores hidden mount points. */
>>> +				goto jump_up;
>>> +			} else {
>>> +				/*
>>> +				 * Stops at the real root.  Denies access
>>> +				 * because not all layers have granted access.
>>> +				 */
>>> +				allowed = false;
>>> +				break;
>>> +			}
>>> +		}
>>> +		if (unlikely(IS_ROOT(walker_path.dentry))) {
>>> +			/*
>>> +			 * Stops at disconnected root directories.  Only allows
>>> +			 * access to internal filesystems (e.g. nsfs, which is
>>> +			 * reachable through /proc/<pid>/ns/<namespace>).
>>> +			 */
>>> +			allowed = !!(walker_path.mnt->mnt_flags & MNT_INTERNAL);
>>> +			break;
>>> +		}
>>> +		parent_dentry = dget_parent(walker_path.dentry);
>>> +		dput(walker_path.dentry);
>>> +		walker_path.dentry = parent_dentry;
>>> +	}
>>> +	path_put(&walker_path);
>>> +	return allowed ? 0 : -EACCES;
> 
> That's a whole lot of grabbing/dropping references...  I realize that it's
> an utterly tactless question, but... how costly it is?  IOW, do you have
> profiling data?

It looks like a legitimate question.

First, Landlock may not be appropriate for every workloads. The
check_access_path()'s complexity is now linear, which is a consequence
of the "unprivileged" target (i.e. multiple layers of file hierarchies).
Adding caching should help a lot to improve performance (i.e. limit the
path walking), but it will come with future improvements.

I profiled a "find" loop on the linux-5.12-rc3 source tree in a tmpfs
(and with cached entries): openat(2) calls spend ~30% of their time in
check_access_path() with a base directory of one parent (/linux) and
~45% with a base directory of ten parents (/1/2/3/4/5/6/7/8/9/linux).
Overall, the performance impact is between 3.0% (with a minimum depth of
1) and 5.4% (with a minimum depth of 10) of the full execution time of
these worse case scenarios, which are ~4800 openat(2) calls. This is not
a surprise and doesn't seem so bad without optimization.


> 
>>> +/*
>>> + * pivot_root(2), like mount(2), changes the current mount namespace.  It must
>>> + * then be forbidden for a landlocked process.
> 
> ... and cross-directory rename(2) can change the tree topology.  Do you ban that
> as well?
> 
> [snip]
> 
>>> +static int hook_path_rename(const struct path *const old_dir,
>>> +		struct dentry *const old_dentry,
>>> +		const struct path *const new_dir,
>>> +		struct dentry *const new_dentry)
>>> +{
>>> +	const struct landlock_ruleset *const dom =
>>> +		landlock_get_current_domain();
>>> +
>>> +	if (!dom)
>>> +		return 0;
>>> +	/* The mount points are the same for old and new paths, cf. EXDEV. */
>>> +	if (old_dir->dentry != new_dir->dentry)
>>> +		/* For now, forbids reparenting. */
>>> +		return -EACCES;
> 
> You do, apparently, and not in a way that would have the userland fall
> back to copy+unlink.  Lovely...  Does e.g. git survive such restriction?
> Same question for your average package build...

As explained in the documentation, there is some limitations that make
this first step not appropriate for all use cases. I'll use EXDEV to
gracefully forbid reparenting, which gives a chance to userspace to deal
with that. It may not be enough for package management though. I plan to
address such limitation with future evolutions.

Thanks for these suggestions.

  reply index

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-24 19:15 [PATCH v31 00/12] Landlock LSM Mickaël Salaün
2021-03-24 19:15 ` [PATCH v31 01/12] landlock: Add object management Mickaël Salaün
2021-03-24 19:34   ` Mickaël Salaün
2021-03-24 19:15 ` [PATCH v31 02/12] landlock: Add ruleset and domain management Mickaël Salaün
2021-03-24 19:15 ` [PATCH v31 03/12] landlock: Set up the security framework and manage credentials Mickaël Salaün
2021-03-24 19:15 ` [PATCH v31 04/12] landlock: Add ptrace restrictions Mickaël Salaün
2021-03-24 19:15 ` [PATCH v31 05/12] LSM: Infrastructure management of the superblock Mickaël Salaün
2021-03-24 19:15 ` [PATCH v31 06/12] fs,security: Add sb_delete hook Mickaël Salaün
2021-03-24 19:15 ` [PATCH v31 07/12] landlock: Support filesystem access-control Mickaël Salaün
2021-03-31 17:33   ` Mickaël Salaün
2021-03-31 17:50     ` Kees Cook
2021-04-01  2:14     ` Al Viro
2021-04-01 17:12       ` Mickaël Salaün [this message]
2021-03-24 19:15 ` [PATCH v31 08/12] landlock: Add syscall implementations Mickaël Salaün
2021-03-24 19:15 ` [PATCH v31 09/12] arch: Wire up Landlock syscalls Mickaël Salaün
2021-03-24 19:15 ` [PATCH v31 10/12] selftests/landlock: Add user space tests Mickaël Salaün
2021-03-26  4:30   ` Kees Cook
2021-03-24 19:15 ` [PATCH v31 11/12] samples/landlock: Add a sandbox manager example Mickaël Salaün
2021-03-24 19:15 ` [PATCH v31 12/12] landlock: Add user and kernel documentation Mickaël Salaün
2021-03-26  4:30   ` Kees Cook

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=84e1cd29-0f09-1ed4-c680-65ca8c6988a3@digikod.net \
    --to=mic@digikod.net \
    --cc=akpm@linux-foundation.org \
    --cc=anton.ivanov@cambridgegreys.com \
    --cc=arnd@arndb.de \
    --cc=casey@schaufler-ca.com \
    --cc=corbet@lwn.net \
    --cc=dhowells@redhat.com \
    --cc=jannh@google.com \
    --cc=jdike@addtoit.com \
    --cc=jmorris@namei.org \
    --cc=keescook@chromium.org \
    --cc=kernel-hardening@lists.openwall.com \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=luto@amacapital.net \
    --cc=mic@linux.microsoft.com \
    --cc=mtk.manpages@gmail.com \
    --cc=richard@nod.at \
    --cc=serge@hallyn.com \
    --cc=shuah@kernel.org \
    --cc=vincent.dagonneau@ssi.gouv.fr \
    --cc=viro@zeniv.linux.org.uk \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-api Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-api/0 linux-api/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-api linux-api/ https://lore.kernel.org/linux-api \
		linux-api@vger.kernel.org
	public-inbox-index linux-api

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-api


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git