Linux-Security-Module Archive on lore.kernel.org
 help / color / Atom feed
From: Al Viro <viro@zeniv.linux.org.uk>
To: "Mickaël Salaün" <mickael.salaun@ssi.gouv.fr>
Cc: "Mickaël Salaün" <mic@digikod.net>,
	linux-kernel@vger.kernel.org, "Aleksa Sarai" <cyphar@cyphar.com>,
	"Alexei Starovoitov" <ast@kernel.org>,
	"Andrew Morton" <akpm@linux-foundation.org>,
	"Andy Lutomirski" <luto@amacapital.net>,
	"Arnaldo Carvalho de Melo" <acme@kernel.org>,
	"Casey Schaufler" <casey@schaufler-ca.com>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	"David Drysdale" <drysdale@google.com>,
	"David S . Miller" <davem@davemloft.net>,
	"Eric W . Biederman" <ebiederm@xmission.com>,
	"James Morris" <jmorris@namei.org>, "Jann Horn" <jann@thejh.net>,
	"John Johansen" <john.johansen@canonical.com>,
	"Jonathan Corbet" <corbet@lwn.net>,
	"Kees Cook" <keescook@chromium.org>,
	"Michael Kerrisk" <mtk.manpages@gmail.com>,
	"Paul Moore" <paul@paul-moore.com>,
	"Sargun Dhillon" <sargun@sargun.me>,
	"Serge E . Hallyn" <serge@hallyn.com>,
	"Shuah Khan" <shuah@kernel.org>,
	"Stephen Smalley" <sds@tycho.nsa.gov>,
	"Tejun Heo" <tj@kernel.org>,
	"Tetsuo Handa" <penguin-kernel@i-love.sakura.ne.jp>,
	"Thomas Graf" <tgraf@suug.ch>, "Tycho Andersen" <tycho@tycho.ws>,
	"Will Drewry" <wad@chromium.org>,
	kernel-hardening@lists.openwall.com, linux-api@vger.kernel.org,
	linux-fsdevel@vger.kernel.org,
	linux-security-module@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [PATCH bpf-next v9 05/10] bpf,landlock: Add a new map type: inode
Date: Thu, 27 Jun 2019 17:56:40 +0100
Message-ID: <20190627165640.GQ17978@ZenIV.linux.org.uk> (raw)
In-Reply-To: <79bac827-4092-8a4d-9dc6-6019419b2486@ssi.gouv.fr>

On Thu, Jun 27, 2019 at 06:18:12PM +0200, Mickaël Salaün wrote:

> >> +/* called from syscall */
> >> +static int sys_inode_map_delete_elem(struct bpf_map *map, struct inode *key)
> >> +{
> >> +    struct inode_array *array = container_of(map, struct inode_array, map);
> >> +    struct inode *inode;
> >> +    int i;
> >> +
> >> +    WARN_ON_ONCE(!rcu_read_lock_held());
> >> +    for (i = 0; i < array->map.max_entries; i++) {
> >> +            if (array->elems[i].inode == key) {
> >> +                    inode = xchg(&array->elems[i].inode, NULL);
> >> +                    array->nb_entries--;
> >
> > Umm...  Is that intended to be atomic in any sense?
> 
> nb_entries is not used as a bound check but to avoid walking uselessly
> through the (pre-allocated) array when adding a new element, but I'll
> use an atomic to avoid inconsistencies anyway.


> > Wait a sec...  So we have those beasties that can have long-term
> > references to arbitrary inodes stuck in them?  What will happen
> > if you get umount(2) called while such a thing exists?
> 
> I though an umount would be denied but no, we get a self-destructed busy
> inode and a bug!
> What about wrapping the inode's superblock->s_op->destroy_inode() to
> first remove the element from the map and then call the real
> destroy_inode(), if any?

What do you mean, _the_ map?  I don't see anything to prevent insertion
of references to the same inode into any number of those...

> Or I could update fs/inode.c:destroy_inode() to call inode->free_inode()
> if it is set, and set it when such inode is referenced by a map?
> Or maybe I could hold the referencing file in the map and then wrap its
> f_op?

First of all, anything including the word "wrap" is a non-starter.
We really don't need the headache associated with the locking needed
to replace the method tables on the fly, or with the code checking that
->f_op points to given method table, etc.  That's not going to fly,
especially since you'd end up _chaining_ those (again, the same reference
can go in more than once).

Nothing is allowed to change the method tables of live objects, period.
Once a struct file is opened, its ->f_op is never going to change and
it entirely belongs to the device driver or filesystem it resides on.
Nothing else (not VFS, not VM, not some LSM module, etc.) has any business
touching that.  The same goes for inodes, dentries, etc.

What kind of behaviour do you want there?  Do you want the inodes you've
referenced there to be forgotten on e.g. memory pressure?  The thing is,
I don't see how "it's getting freed" could map onto any semantics that
might be useful for you - it looks like the wrong event for that.

  reply index

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-25 21:52 [PATCH bpf-next v9 00/10] Landlock LSM: Toward unprivileged sandboxing Mickaël Salaün
2019-06-25 21:52 ` [PATCH bpf-next v9 01/10] fs,security: Add a new file access type: MAY_CHROOT Mickaël Salaün
2019-06-25 21:52 ` [PATCH bpf-next v9 02/10] bpf: Add eBPF program subtype and is_valid_subtype() verifier Mickaël Salaün
2019-06-25 23:02   ` Alexei Starovoitov
2019-06-26  7:33     ` Mickaël Salaün
2019-06-25 21:52 ` [PATCH bpf-next v9 03/10] bpf,landlock: Define an eBPF program type for Landlock hooks Mickaël Salaün
2019-06-25 21:52 ` [PATCH bpf-next v9 04/10] seccomp,landlock: Enforce Landlock programs per process hierarchy Mickaël Salaün
2019-06-25 21:52 ` [PATCH bpf-next v9 05/10] bpf,landlock: Add a new map type: inode Mickaël Salaün
2019-06-25 22:52   ` Al Viro
2019-06-27 16:18     ` Mickaël Salaün
2019-06-27 16:56       ` Al Viro [this message]
2019-06-28 13:17         ` Mickaël Salaün
2019-06-25 21:52 ` [PATCH bpf-next v9 06/10] landlock: Handle filesystem access control Mickaël Salaün
2019-06-25 21:52 ` [PATCH bpf-next v9 07/10] landlock: Add ptrace restrictions Mickaël Salaün
2019-06-25 21:52 ` [PATCH bpf-next v9 08/10] bpf: Add a Landlock sandbox example Mickaël Salaün
2019-06-25 21:52 ` [PATCH bpf-next v9 09/10] bpf,landlock: Add tests for Landlock Mickaël Salaün
2019-06-25 21:52 ` [PATCH bpf-next v9 10/10] landlock: Add user and kernel documentation " Mickaël Salaün

Reply instructions:

You may reply publically 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=20190627165640.GQ17978@ZenIV.linux.org.uk \
    --to=viro@zeniv.linux.org.uk \
    --cc=acme@kernel.org \
    --cc=akpm@linux-foundation.org \
    --cc=ast@kernel.org \
    --cc=casey@schaufler-ca.com \
    --cc=corbet@lwn.net \
    --cc=cyphar@cyphar.com \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=drysdale@google.com \
    --cc=ebiederm@xmission.com \
    --cc=jann@thejh.net \
    --cc=jmorris@namei.org \
    --cc=john.johansen@canonical.com \
    --cc=keescook@chromium.org \
    --cc=kernel-hardening@lists.openwall.com \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=luto@amacapital.net \
    --cc=mic@digikod.net \
    --cc=mickael.salaun@ssi.gouv.fr \
    --cc=mtk.manpages@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=paul@paul-moore.com \
    --cc=penguin-kernel@i-love.sakura.ne.jp \
    --cc=sargun@sargun.me \
    --cc=sds@tycho.nsa.gov \
    --cc=serge@hallyn.com \
    --cc=shuah@kernel.org \
    --cc=tgraf@suug.ch \
    --cc=tj@kernel.org \
    --cc=tycho@tycho.ws \
    --cc=wad@chromium.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-Security-Module Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-security-module/0 linux-security-module/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-security-module linux-security-module/ https://lore.kernel.org/linux-security-module \
		linux-security-module@vger.kernel.org linux-security-module@archiver.kernel.org
	public-inbox-index linux-security-module


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


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