From: Al Viro <viro@zeniv.linux.org.uk> To: "Mickaël Salaün" <mic@digikod.net> Cc: 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>, "Mickaël Salaün" <mickael.salaun@ssi.gouv.fr>, "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: Tue, 25 Jun 2019 23:52:01 +0100 [thread overview] Message-ID: <20190625225201.GJ17978@ZenIV.linux.org.uk> (raw) In-Reply-To: <20190625215239.11136-6-mic@digikod.net> On Tue, Jun 25, 2019 at 11:52:34PM +0200, Mickaël Salaün wrote: > +/* must call iput(inode) after this call */ > +static struct inode *inode_from_fd(int ufd, bool check_access) > +{ > + struct inode *ret; > + struct fd f; > + int deny; > + > + f = fdget(ufd); > + if (unlikely(!f.file || !file_inode(f.file))) { > + ret = ERR_PTR(-EBADF); > + goto put_fd; > + } Just when does one get a NULL file_inode()? The reason I'm asking is that arseloads of code would break if one managed to create such a beast... Incidentally, that should be return ERR_PTR(-EBADF); fdput() is wrong there. > + } > + /* check if the FD is tied to a mount point */ > + /* TODO: add this check when called from an eBPF program too */ > + if (unlikely(!f.file->f_path.mnt Again, the same question - when the hell can that happen? If you are sitting on an exploitable roothole, do share it... || f.file->f_path.mnt->mnt_flags & > + MNT_INTERNAL)) { > + ret = ERR_PTR(-EINVAL); > + goto put_fd; What does it have to do with mountpoints, anyway? > +/* 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? > + iput(inode); > + return 0; > + } > + } > + return -ENOENT; > +} > + > +/* called from syscall */ > +int bpf_inode_map_delete_elem(struct bpf_map *map, int *key) > +{ > + struct inode *inode; > + int err; > + > + inode = inode_from_fd(*key, false); > + if (IS_ERR(inode)) > + return PTR_ERR(inode); > + err = sys_inode_map_delete_elem(map, inode); > + iput(inode); > + return err; > +} 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?
WARNING: multiple messages have this Message-ID (diff)
From: Al Viro <viro@zeniv.linux.org.uk> To: "Mickaël Salaün" <mic@digikod.net> Cc: 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>, "Mickaël Salaün" <mickael.salaun@ssi.gouv.fr>, "Paul Moore" <paul@paul-moore.com> Subject: Re: [PATCH bpf-next v9 05/10] bpf,landlock: Add a new map type: inode Date: Tue, 25 Jun 2019 23:52:01 +0100 [thread overview] Message-ID: <20190625225201.GJ17978@ZenIV.linux.org.uk> (raw) In-Reply-To: <20190625215239.11136-6-mic@digikod.net> On Tue, Jun 25, 2019 at 11:52:34PM +0200, Mickaël Salaün wrote: > +/* must call iput(inode) after this call */ > +static struct inode *inode_from_fd(int ufd, bool check_access) > +{ > + struct inode *ret; > + struct fd f; > + int deny; > + > + f = fdget(ufd); > + if (unlikely(!f.file || !file_inode(f.file))) { > + ret = ERR_PTR(-EBADF); > + goto put_fd; > + } Just when does one get a NULL file_inode()? The reason I'm asking is that arseloads of code would break if one managed to create such a beast... Incidentally, that should be return ERR_PTR(-EBADF); fdput() is wrong there. > + } > + /* check if the FD is tied to a mount point */ > + /* TODO: add this check when called from an eBPF program too */ > + if (unlikely(!f.file->f_path.mnt Again, the same question - when the hell can that happen? If you are sitting on an exploitable roothole, do share it... || f.file->f_path.mnt->mnt_flags & > + MNT_INTERNAL)) { > + ret = ERR_PTR(-EINVAL); > + goto put_fd; What does it have to do with mountpoints, anyway? > +/* 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? > + iput(inode); > + return 0; > + } > + } > + return -ENOENT; > +} > + > +/* called from syscall */ > +int bpf_inode_map_delete_elem(struct bpf_map *map, int *key) > +{ > + struct inode *inode; > + int err; > + > + inode = inode_from_fd(*key, false); > + if (IS_ERR(inode)) > + return PTR_ERR(inode); > + err = sys_inode_map_delete_elem(map, inode); > + iput(inode); > + return err; > +} 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?
next prev parent reply other threads:[~2019-06-25 22:52 UTC|newest] Thread overview: 34+ 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 ` 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 ` 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 21:52 ` Mickaël Salaün 2019-06-25 23:02 ` Alexei Starovoitov 2019-06-25 23:02 ` Alexei Starovoitov 2019-06-26 7:33 ` Mickaël Salaün 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 ` 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 ` 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 21:52 ` Mickaël Salaün 2019-06-25 22:52 ` Al Viro [this message] 2019-06-25 22:52 ` Al Viro 2019-06-27 16:18 ` Mickaël Salaün 2019-06-27 16:18 ` Mickaël Salaün 2019-06-27 16:56 ` Al Viro 2019-06-27 16:56 ` Al Viro 2019-06-28 13:17 ` Mickaël Salaün 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 ` 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 ` 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 ` 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 ` 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 2019-06-25 21:52 ` Mickaël Salaün
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=20190625225201.GJ17978@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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.