* Re: [PATCH ghak90 V6 02/10] audit: add container id
From: Paul Moore @ 2019-07-15 21:09 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Tycho Andersen, Serge E. Hallyn, containers, linux-api,
Linux-Audit Mailing List, linux-fsdevel, LKML, netdev,
netfilter-devel, sgrubb, omosnace, dhowells, simo, Eric Paris,
ebiederm, nhorman
In-Reply-To: <20190708181237.5poheliito7zpvmc@madcap2.tricolour.ca>
On Mon, Jul 8, 2019 at 2:12 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2019-05-30 19:26, Paul Moore wrote:
...
> > I like the creativity, but I worry that at some point these
> > limitations are going to be raised (limits have a funny way of doing
> > that over time) and we will be in trouble. I say "trouble" because I
> > want to be able to quickly do an audit container ID comparison and
> > we're going to pay a penalty for these larger values (we'll need this
> > when we add multiple auditd support and the requisite record routing).
> >
> > Thinking about this makes me also realize we probably need to think a
> > bit longer about audit container ID conflicts between orchestrators.
> > Right now we just take the value that is given to us by the
> > orchestrator, but if we want to allow multiple container orchestrators
> > to work without some form of cooperation in userspace (I think we have
> > to assume the orchestrators will not talk to each other) we likely
> > need to have some way to block reuse of an audit container ID. We
> > would either need to prevent the orchestrator from explicitly setting
> > an audit container ID to a currently in use value, or instead generate
> > the audit container ID in the kernel upon an event triggered by the
> > orchestrator (e.g. a write to a /proc file). I suspect we should
> > start looking at the idr code, I think we will need to make use of it.
>
> To address this, I'd suggest that it is enforced to only allow the
> setting of descendants and to maintain a master list of audit container
> identifiers (with a hash table if necessary later) that includes the
> container owner.
We're discussing the audit container ID management policy elsewhere in
this thread so I won't comment on that here, but I did want to say
that we will likely need something better than a simple list of audit
container IDs from the start. It's common for systems to have
thousands of containers now (or multiple thousands), which tells me
that a list is a poor choice. You mentioned a hash table, so I would
suggest starting with that over the list for the initial patchset.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH V35 23/29] bpf: Restrict bpf when kernel lockdown is in confidentiality mode
From: Daniel Borkmann @ 2019-07-15 22:54 UTC (permalink / raw)
To: Matthew Garrett, jmorris
Cc: linux-security-module, linux-kernel, linux-api, David Howells,
Alexei Starovoitov, Matthew Garrett, netdev, Chun-Yi Lee
In-Reply-To: <20190715195946.223443-24-matthewgarrett@google.com>
On 7/15/19 9:59 PM, Matthew Garrett wrote:
> From: David Howells <dhowells@redhat.com>
>
> bpf_read() and bpf_read_str() could potentially be abused to (eg) allow
> private keys in kernel memory to be leaked. Disable them if the kernel
> has been locked down in confidentiality mode.
>
> Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> cc: netdev@vger.kernel.org
> cc: Chun-Yi Lee <jlee@suse.com>
> cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> ---
> include/linux/security.h | 1 +
> kernel/trace/bpf_trace.c | 10 ++++++++++
> security/lockdown/lockdown.c | 1 +
> 3 files changed, 12 insertions(+)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 987d8427f091..8dd1741a52cd 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -118,6 +118,7 @@ enum lockdown_reason {
> LOCKDOWN_INTEGRITY_MAX,
> LOCKDOWN_KCORE,
> LOCKDOWN_KPROBES,
> + LOCKDOWN_BPF_READ,
> LOCKDOWN_CONFIDENTIALITY_MAX,
> };
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index ca1255d14576..605908da61c5 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -142,7 +142,12 @@ BPF_CALL_3(bpf_probe_read, void *, dst, u32, size, const void *, unsafe_ptr)
> {
> int ret;
>
> + ret = security_locked_down(LOCKDOWN_BPF_READ);
> + if (ret)
> + goto out;
> +
> ret = probe_kernel_read(dst, unsafe_ptr, size);
> +out:
> if (unlikely(ret < 0))
> memset(dst, 0, size);
Hmm, does security_locked_down() ever return a code > 0 or why do you
have the double check on return code? If not, then for clarity the
ret code from security_locked_down() should be checked as 'ret < 0'
as well and out label should be at the memset directly instead.
> @@ -569,6 +574,10 @@ BPF_CALL_3(bpf_probe_read_str, void *, dst, u32, size,
> {
> int ret;
>
> + ret = security_locked_down(LOCKDOWN_BPF_READ);
> + if (ret)
> + goto out;
> +
> /*
> * The strncpy_from_unsafe() call will likely not fill the entire
> * buffer, but that's okay in this circumstance as we're probing
> @@ -579,6 +588,7 @@ BPF_CALL_3(bpf_probe_read_str, void *, dst, u32, size,
> * is returned that can be used for bpf_perf_event_output() et al.
> */
> ret = strncpy_from_unsafe(dst, unsafe_ptr, size);
> +out:
> if (unlikely(ret < 0))
> memset(dst, 0, size);
Ditto.
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH V35 26/29] debugfs: Restrict debugfs when the kernel is locked down
From: James Morris @ 2019-07-15 23:17 UTC (permalink / raw)
To: Matthew Garrett, Greg KH, rafael
Cc: linux-security-module, linux-kernel, linux-api, David Howells,
Andy Shevchenko, acpi4asus-user, platform-driver-x86,
Matthew Garrett, Thomas Gleixner
In-Reply-To: <20190715195946.223443-27-matthewgarrett@google.com>
On Mon, 15 Jul 2019, Matthew Garrett wrote:
> From: David Howells <dhowells@redhat.com>
>
> Disallow opening of debugfs files that might be used to muck around when
> the kernel is locked down as various drivers give raw access to hardware
> through debugfs. Given the effort of auditing all 2000 or so files and
> manually fixing each one as necessary, I've chosen to apply a heuristic
> instead. The following changes are made:
Adding the debugfs maintainers.
>
> (1) chmod and chown are disallowed on debugfs objects (though the root dir
> can be modified by mount and remount, but I'm not worried about that).
>
> (2) When the kernel is locked down, only files with the following criteria
> are permitted to be opened:
>
> - The file must have mode 00444
> - The file must not have ioctl methods
> - The file must not have mmap
>
> (3) When the kernel is locked down, files may only be opened for reading.
>
> Normal device interaction should be done through configfs, sysfs or a
> miscdev, not debugfs.
>
> Note that this makes it unnecessary to specifically lock down show_dsts(),
> show_devs() and show_call() in the asus-wmi driver.
>
> I would actually prefer to lock down all files by default and have the
> the files unlocked by the creator. This is tricky to manage correctly,
> though, as there are 19 creation functions and ~1600 call sites (some of
> them in loops scanning tables).
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> cc: acpi4asus-user@lists.sourceforge.net
> cc: platform-driver-x86@vger.kernel.org
> cc: Matthew Garrett <mjg59@srcf.ucam.org>
> cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Matthew Garrett <matthewgarrett@google.com>
> ---
> fs/debugfs/file.c | 30 ++++++++++++++++++++++++++++++
> fs/debugfs/inode.c | 32 ++++++++++++++++++++++++++++++--
> include/linux/security.h | 1 +
> security/lockdown/lockdown.c | 1 +
> 4 files changed, 62 insertions(+), 2 deletions(-)
>
> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> index 93e4ca6b2ad7..87846aad594b 100644
> --- a/fs/debugfs/file.c
> +++ b/fs/debugfs/file.c
> @@ -19,6 +19,7 @@
> #include <linux/atomic.h>
> #include <linux/device.h>
> #include <linux/poll.h>
> +#include <linux/security.h>
>
> #include "internal.h"
>
> @@ -136,6 +137,25 @@ void debugfs_file_put(struct dentry *dentry)
> }
> EXPORT_SYMBOL_GPL(debugfs_file_put);
>
> +/*
> + * Only permit access to world-readable files when the kernel is locked down.
> + * We also need to exclude any file that has ways to write or alter it as root
> + * can bypass the permissions check.
> + */
> +static bool debugfs_is_locked_down(struct inode *inode,
> + struct file *filp,
> + const struct file_operations *real_fops)
> +{
> + if ((inode->i_mode & 07777) == 0444 &&
> + !(filp->f_mode & FMODE_WRITE) &&
> + !real_fops->unlocked_ioctl &&
> + !real_fops->compat_ioctl &&
> + !real_fops->mmap)
> + return false;
> +
> + return security_locked_down(LOCKDOWN_DEBUGFS);
> +}
> +
> static int open_proxy_open(struct inode *inode, struct file *filp)
> {
> struct dentry *dentry = F_DENTRY(filp);
> @@ -147,6 +167,11 @@ static int open_proxy_open(struct inode *inode, struct file *filp)
> return r == -EIO ? -ENOENT : r;
>
> real_fops = debugfs_real_fops(filp);
> +
> + r = debugfs_is_locked_down(inode, filp, real_fops);
> + if (r)
> + goto out;
> +
> real_fops = fops_get(real_fops);
> if (!real_fops) {
> /* Huh? Module did not clean up after itself at exit? */
> @@ -272,6 +297,11 @@ static int full_proxy_open(struct inode *inode, struct file *filp)
> return r == -EIO ? -ENOENT : r;
>
> real_fops = debugfs_real_fops(filp);
> +
> + r = debugfs_is_locked_down(inode, filp, real_fops);
> + if (r)
> + goto out;
> +
> real_fops = fops_get(real_fops);
> if (!real_fops) {
> /* Huh? Module did not cleanup after itself at exit? */
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index 042b688ed124..7b975dbb2bb4 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -26,6 +26,7 @@
> #include <linux/parser.h>
> #include <linux/magic.h>
> #include <linux/slab.h>
> +#include <linux/security.h>
>
> #include "internal.h"
>
> @@ -35,6 +36,32 @@ static struct vfsmount *debugfs_mount;
> static int debugfs_mount_count;
> static bool debugfs_registered;
>
> +/*
> + * Don't allow access attributes to be changed whilst the kernel is locked down
> + * so that we can use the file mode as part of a heuristic to determine whether
> + * to lock down individual files.
> + */
> +static int debugfs_setattr(struct dentry *dentry, struct iattr *ia)
> +{
> + int ret = security_locked_down(LOCKDOWN_DEBUGFS);
> +
> + if (ret && (ia->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID)))
> + return ret;
> + return simple_setattr(dentry, ia);
> +}
> +
> +static const struct inode_operations debugfs_file_inode_operations = {
> + .setattr = debugfs_setattr,
> +};
> +static const struct inode_operations debugfs_dir_inode_operations = {
> + .lookup = simple_lookup,
> + .setattr = debugfs_setattr,
> +};
> +static const struct inode_operations debugfs_symlink_inode_operations = {
> + .get_link = simple_get_link,
> + .setattr = debugfs_setattr,
> +};
> +
> static struct inode *debugfs_get_inode(struct super_block *sb)
> {
> struct inode *inode = new_inode(sb);
> @@ -369,6 +396,7 @@ static struct dentry *__debugfs_create_file(const char *name, umode_t mode,
> inode->i_mode = mode;
> inode->i_private = data;
>
> + inode->i_op = &debugfs_file_inode_operations;
> inode->i_fop = proxy_fops;
> dentry->d_fsdata = (void *)((unsigned long)real_fops |
> DEBUGFS_FSDATA_IS_REAL_FOPS_BIT);
> @@ -532,7 +560,7 @@ struct dentry *debugfs_create_dir(const char *name, struct dentry *parent)
> }
>
> inode->i_mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO;
> - inode->i_op = &simple_dir_inode_operations;
> + inode->i_op = &debugfs_dir_inode_operations;
> inode->i_fop = &simple_dir_operations;
>
> /* directory inodes start off with i_nlink == 2 (for "." entry) */
> @@ -632,7 +660,7 @@ struct dentry *debugfs_create_symlink(const char *name, struct dentry *parent,
> return failed_creating(dentry);
> }
> inode->i_mode = S_IFLNK | S_IRWXUGO;
> - inode->i_op = &simple_symlink_inode_operations;
> + inode->i_op = &debugfs_symlink_inode_operations;
> inode->i_link = link;
> d_instantiate(dentry, inode);
> return end_creating(dentry);
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 8ef366de70b0..d92323b44a3f 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -115,6 +115,7 @@ enum lockdown_reason {
> LOCKDOWN_TIOCSSERIAL,
> LOCKDOWN_MODULE_PARAMETERS,
> LOCKDOWN_MMIOTRACE,
> + LOCKDOWN_DEBUGFS,
> LOCKDOWN_INTEGRITY_MAX,
> LOCKDOWN_KCORE,
> LOCKDOWN_KPROBES,
> diff --git a/security/lockdown/lockdown.c b/security/lockdown/lockdown.c
> index e43c9d001e49..37ef46320ef4 100644
> --- a/security/lockdown/lockdown.c
> +++ b/security/lockdown/lockdown.c
> @@ -30,6 +30,7 @@ static char *lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = {
> [LOCKDOWN_TIOCSSERIAL] = "reconfiguration of serial port IO",
> [LOCKDOWN_MODULE_PARAMETERS] = "unsafe module parameters",
> [LOCKDOWN_MMIOTRACE] = "unsafe mmio",
> + [LOCKDOWN_DEBUGFS] = "debugfs access",
> [LOCKDOWN_INTEGRITY_MAX] = "integrity",
> [LOCKDOWN_KCORE] = "/proc/kcore access",
> [LOCKDOWN_KPROBES] = "use of kprobes",
>
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH V35 19/29] Lock down module params that specify hardware parameters (eg. ioport)
From: James Morris @ 2019-07-15 23:26 UTC (permalink / raw)
To: Matthew Garrett, jeyu
Cc: linux-security-module, linux-kernel, linux-api, David Howells,
Alan Cox, Matthew Garrett, Kees Cook
In-Reply-To: <20190715195946.223443-20-matthewgarrett@google.com>
On Mon, 15 Jul 2019, Matthew Garrett wrote:
> From: David Howells <dhowells@redhat.com>
>
> Provided an annotation for module parameters that specify hardware
> parameters (such as io ports, iomem addresses, irqs, dma channels, fixed
> dma buffers and other types).
Adding Jessica.
>
> Suggested-by: Alan Cox <gnomes@lxorguk.ukuu.org.uk>
> Signed-off-by: David Howells <dhowells@redhat.com>
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> ---
> include/linux/security.h | 1 +
> kernel/params.c | 28 +++++++++++++++++++++++-----
> security/lockdown/lockdown.c | 1 +
> 3 files changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 8f7048395114..43fa3486522b 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -113,6 +113,7 @@ enum lockdown_reason {
> LOCKDOWN_ACPI_TABLES,
> LOCKDOWN_PCMCIA_CIS,
> LOCKDOWN_TIOCSSERIAL,
> + LOCKDOWN_MODULE_PARAMETERS,
> LOCKDOWN_INTEGRITY_MAX,
> LOCKDOWN_CONFIDENTIALITY_MAX,
> };
> diff --git a/kernel/params.c b/kernel/params.c
> index cf448785d058..f2779a76d39a 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -12,6 +12,7 @@
> #include <linux/err.h>
> #include <linux/slab.h>
> #include <linux/ctype.h>
> +#include <linux/security.h>
>
> #ifdef CONFIG_SYSFS
> /* Protects all built-in parameters, modules use their own param_lock */
> @@ -96,13 +97,20 @@ bool parameq(const char *a, const char *b)
> return parameqn(a, b, strlen(a)+1);
> }
>
> -static void param_check_unsafe(const struct kernel_param *kp)
> +static bool param_check_unsafe(const struct kernel_param *kp,
> + const char *doing)
> {
> + if (kp->flags & KERNEL_PARAM_FL_HWPARAM &&
> + security_locked_down(LOCKDOWN_MODULE_PARAMETERS))
> + return false;
> +
> if (kp->flags & KERNEL_PARAM_FL_UNSAFE) {
> pr_notice("Setting dangerous option %s - tainting kernel\n",
> kp->name);
> add_taint(TAINT_USER, LOCKDEP_STILL_OK);
> }
> +
> + return true;
> }
>
> static int parse_one(char *param,
> @@ -132,8 +140,10 @@ static int parse_one(char *param,
> pr_debug("handling %s with %p\n", param,
> params[i].ops->set);
> kernel_param_lock(params[i].mod);
> - param_check_unsafe(¶ms[i]);
> - err = params[i].ops->set(val, ¶ms[i]);
> + if (param_check_unsafe(¶ms[i], doing))
> + err = params[i].ops->set(val, ¶ms[i]);
> + else
> + err = -EPERM;
> kernel_param_unlock(params[i].mod);
> return err;
> }
> @@ -541,6 +551,12 @@ static ssize_t param_attr_show(struct module_attribute *mattr,
> return count;
> }
>
> +#ifdef CONFIG_MODULES
> +#define mod_name(mod) ((mod)->name)
> +#else
> +#define mod_name(mod) "unknown"
> +#endif
> +
> /* sysfs always hands a nul-terminated string in buf. We rely on that. */
> static ssize_t param_attr_store(struct module_attribute *mattr,
> struct module_kobject *mk,
> @@ -553,8 +569,10 @@ static ssize_t param_attr_store(struct module_attribute *mattr,
> return -EPERM;
>
> kernel_param_lock(mk->mod);
> - param_check_unsafe(attribute->param);
> - err = attribute->param->ops->set(buf, attribute->param);
> + if (param_check_unsafe(attribute->param, mod_name(mk->mod)))
> + err = attribute->param->ops->set(buf, attribute->param);
> + else
> + err = -EPERM;
> kernel_param_unlock(mk->mod);
> if (!err)
> return len;
> diff --git a/security/lockdown/lockdown.c b/security/lockdown/lockdown.c
> index 07a49667f234..065432f9e218 100644
> --- a/security/lockdown/lockdown.c
> +++ b/security/lockdown/lockdown.c
> @@ -28,6 +28,7 @@ static char *lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = {
> [LOCKDOWN_ACPI_TABLES] = "modified ACPI tables",
> [LOCKDOWN_PCMCIA_CIS] = "direct PCMCIA CIS storage",
> [LOCKDOWN_TIOCSSERIAL] = "reconfiguration of serial port IO",
> + [LOCKDOWN_MODULE_PARAMETERS] = "unsafe module parameters",
> [LOCKDOWN_INTEGRITY_MAX] = "integrity",
> [LOCKDOWN_CONFIDENTIALITY_MAX] = "confidentiality",
> };
>
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH V35 15/29] acpi: Ignore acpi_rsdp kernel param when the kernel has been locked down
From: Dave Young @ 2019-07-16 2:59 UTC (permalink / raw)
To: Matthew Garrett
Cc: jmorris, linux-security-module, linux-kernel, linux-api,
Josh Boyer, David Howells, Matthew Garrett, Borislav Petkov,
Kees Cook, linux-acpi
In-Reply-To: <20190715195946.223443-16-matthewgarrett@google.com>
Hi,
On 07/15/19 at 12:59pm, Matthew Garrett wrote:
> From: Josh Boyer <jwboyer@redhat.com>
>
> This option allows userspace to pass the RSDP address to the kernel, which
> makes it possible for a user to modify the workings of hardware . Reject
> the option when the kernel is locked down.
>
> Signed-off-by: Josh Boyer <jwboyer@redhat.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> cc: Dave Young <dyoung@redhat.com>
> cc: linux-acpi@vger.kernel.org
> ---
> drivers/acpi/osl.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 9c0edf2fc0dd..06e7cffc4386 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -26,6 +26,7 @@
> #include <linux/list.h>
> #include <linux/jiffies.h>
> #include <linux/semaphore.h>
> +#include <linux/security.h>
>
> #include <asm/io.h>
> #include <linux/uaccess.h>
> @@ -180,7 +181,7 @@ acpi_physical_address __init acpi_os_get_root_pointer(void)
> acpi_physical_address pa;
>
> #ifdef CONFIG_KEXEC
> - if (acpi_rsdp)
> + if (acpi_rsdp && !security_locked_down(LOCKDOWN_ACPI_TABLES))
> return acpi_rsdp;
I'm very sorry I noticed this late, but have to say this will not work for
X86 with latest kernel code.
acpi_physical_address __init acpi_os_get_root_pointer(void)
{
acpi_physical_address pa;
#ifdef CONFIG_KEXEC
if (acpi_rsdp)
return acpi_rsdp;
#endif
pa = acpi_arch_get_root_pointer();
if (pa)
return pa;
[snip]
In above code, the later acpi_arch_get_root_pointer still get acpi_rsdp
from cmdline param if provided.
You can check the arch/x86/boot/compressed/acpi.c, and
arch/x86/kernel/acpi/boot.c
In X86 early code, cmdline provided acpi_rsdp pointer will be saved in boot_params.acpi_rsdp_addr;
and the used in x86_default_get_root_pointer
> #endif
> pa = acpi_arch_get_root_pointer();
> --
> 2.22.0.510.g264f2c817a-goog
>
Thanks
Dave
^ permalink raw reply
* Re: [PATCH v9 05/10] namei: O_BENEATH-style path resolution flags
From: Aleksa Sarai @ 2019-07-16 8:03 UTC (permalink / raw)
To: Al Viro
Cc: Jeff Layton, J. Bruce Fields, Arnd Bergmann, David Howells,
Shuah Khan, Shuah Khan, Christian Brauner, David Drysdale,
Andy Lutomirski, Linus Torvalds, Eric Biederman, Andrew Morton,
Alexei Starovoitov, Kees Cook, Jann Horn, Tycho Andersen,
Chanho Min, Oleg Nesterov, Aleksa Sarai, containers, linux-alpha
In-Reply-To: <20190714035826.GQ17978@ZenIV.linux.org.uk>
[-- Attachment #1: Type: text/plain, Size: 2482 bytes --]
On 2019-07-14, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Sat, Jul 13, 2019 at 03:41:53AM +0100, Al Viro wrote:
> > On Fri, Jul 12, 2019 at 04:00:26PM +0100, Al Viro wrote:
> > > On Fri, Jul 12, 2019 at 02:25:53PM +0100, Al Viro wrote:
> > >
> > > > if (flags & LOOKUP_BENEATH) {
> > > > nd->root = nd->path;
> > > > if (!(flags & LOOKUP_RCU))
> > > > path_get(&nd->root);
> > > > else
> > > > nd->root_seq = nd->seq;
> > >
> > > BTW, this assignment is needed for LOOKUP_RCU case. Without it
> > > you are pretty much guaranteed that lazy pathwalk will fail,
> > > when it comes to complete_walk().
> > >
> > > Speaking of which, what would happen if LOOKUP_ROOT/LOOKUP_BENEATH
> > > combination would someday get passed?
> >
> > I don't understand what's going on with ->r_seq in there - your
> > call of path_is_under() is after having (re-)sampled rename_lock,
> > but if that was the only .. in there, who's going to recheck
> > the value? For that matter, what's to guarantee that the thing
> > won't get moved just as you are returning from handle_dots()?
> >
> > IOW, what does LOOKUP_IN_ROOT guarantee for caller (openat2())?
>
> Sigh... Usual effects of trying to document things:
>
> 1) LOOKUP_NO_EVAL looks bogus. It had been introduced by commit 57d4657716ac
> (audit: ignore fcaps on umount) and AFAICS it's crap. It is set in
> ksys_umount() and nowhere else. It's ignored by everything except
> filename_mountpoint(). The thing is, call graph for filename_mountpoint()
> is
> filename_mountpoint()
> <- user_path_mountpoint_at()
> <- ksys_umount()
> <- kern_path_mountpoint()
> <- autofs_dev_ioctl_ismountpoint()
> <- find_autofs_mount()
> <- autofs_dev_ioctl_open_mountpoint()
> <- autofs_dev_ioctl_requester()
> <- autofs_dev_ioctl_ismountpoint()
> In other words, that flag is basically "was filename_mountpoint()
> been called by umount(2) or has it come from an autofs ioctl?".
> And looking at the rationale in that commit, autofs ioctls need
> it just as much as umount(2) does. Why is it not set for those
> as well? And why is it conditional at all?
In addition, LOOKUP_NO_EVAL == LOOKUP_OPEN (0x100). Is that meant to be
the case? Also I just saw you have a patch in work.namei that fixes this
up -- do you want me to rebase on top of that?
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH ghak90 V6 02/10] audit: add container id
From: Richard Guy Briggs @ 2019-07-16 15:37 UTC (permalink / raw)
To: Paul Moore
Cc: Tycho Andersen, Serge E. Hallyn, containers, linux-api,
Linux-Audit Mailing List, linux-fsdevel, LKML, netdev,
netfilter-devel, sgrubb, omosnace, dhowells, simo, Eric Paris,
ebiederm, nhorman
In-Reply-To: <CAHC9VhT0V+xi_6nAR5TsM2vs34LbgMeO=-W+MS_kqiXRRzneZQ@mail.gmail.com>
On 2019-07-15 17:09, Paul Moore wrote:
> On Mon, Jul 8, 2019 at 2:12 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2019-05-30 19:26, Paul Moore wrote:
>
> ...
>
> > > I like the creativity, but I worry that at some point these
> > > limitations are going to be raised (limits have a funny way of doing
> > > that over time) and we will be in trouble. I say "trouble" because I
> > > want to be able to quickly do an audit container ID comparison and
> > > we're going to pay a penalty for these larger values (we'll need this
> > > when we add multiple auditd support and the requisite record routing).
> > >
> > > Thinking about this makes me also realize we probably need to think a
> > > bit longer about audit container ID conflicts between orchestrators.
> > > Right now we just take the value that is given to us by the
> > > orchestrator, but if we want to allow multiple container orchestrators
> > > to work without some form of cooperation in userspace (I think we have
> > > to assume the orchestrators will not talk to each other) we likely
> > > need to have some way to block reuse of an audit container ID. We
> > > would either need to prevent the orchestrator from explicitly setting
> > > an audit container ID to a currently in use value, or instead generate
> > > the audit container ID in the kernel upon an event triggered by the
> > > orchestrator (e.g. a write to a /proc file). I suspect we should
> > > start looking at the idr code, I think we will need to make use of it.
> >
> > To address this, I'd suggest that it is enforced to only allow the
> > setting of descendants and to maintain a master list of audit container
> > identifiers (with a hash table if necessary later) that includes the
> > container owner.
>
> We're discussing the audit container ID management policy elsewhere in
> this thread so I won't comment on that here, but I did want to say
> that we will likely need something better than a simple list of audit
> container IDs from the start. It's common for systems to have
> thousands of containers now (or multiple thousands), which tells me
> that a list is a poor choice. You mentioned a hash table, so I would
> suggest starting with that over the list for the initial patchset.
I saw that as an internal incremental improvement that did not affect
the API, so I wanted to keep things a bit simpler (as you've requested
in the past) to get this going, and add that enhancement later.
I'll start working on it now. The hash table would simply point to
lists anyways unless you can recommend a better approach.
> paul moore
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: [PATCH ghak90 V6 02/10] audit: add container id
From: Paul Moore @ 2019-07-16 16:08 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Tycho Andersen, Serge E. Hallyn, containers, linux-api,
Linux-Audit Mailing List, linux-fsdevel, LKML, netdev,
netfilter-devel, sgrubb, omosnace, dhowells, simo, Eric Paris,
ebiederm, nhorman
In-Reply-To: <20190716153705.xx7dwrhliny5amut@madcap2.tricolour.ca>
On Tue, Jul 16, 2019 at 11:37 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2019-07-15 17:09, Paul Moore wrote:
> > On Mon, Jul 8, 2019 at 2:12 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > On 2019-05-30 19:26, Paul Moore wrote:
> >
> > ...
> >
> > > > I like the creativity, but I worry that at some point these
> > > > limitations are going to be raised (limits have a funny way of doing
> > > > that over time) and we will be in trouble. I say "trouble" because I
> > > > want to be able to quickly do an audit container ID comparison and
> > > > we're going to pay a penalty for these larger values (we'll need this
> > > > when we add multiple auditd support and the requisite record routing).
> > > >
> > > > Thinking about this makes me also realize we probably need to think a
> > > > bit longer about audit container ID conflicts between orchestrators.
> > > > Right now we just take the value that is given to us by the
> > > > orchestrator, but if we want to allow multiple container orchestrators
> > > > to work without some form of cooperation in userspace (I think we have
> > > > to assume the orchestrators will not talk to each other) we likely
> > > > need to have some way to block reuse of an audit container ID. We
> > > > would either need to prevent the orchestrator from explicitly setting
> > > > an audit container ID to a currently in use value, or instead generate
> > > > the audit container ID in the kernel upon an event triggered by the
> > > > orchestrator (e.g. a write to a /proc file). I suspect we should
> > > > start looking at the idr code, I think we will need to make use of it.
> > >
> > > To address this, I'd suggest that it is enforced to only allow the
> > > setting of descendants and to maintain a master list of audit container
> > > identifiers (with a hash table if necessary later) that includes the
> > > container owner.
> >
> > We're discussing the audit container ID management policy elsewhere in
> > this thread so I won't comment on that here, but I did want to say
> > that we will likely need something better than a simple list of audit
> > container IDs from the start. It's common for systems to have
> > thousands of containers now (or multiple thousands), which tells me
> > that a list is a poor choice. You mentioned a hash table, so I would
> > suggest starting with that over the list for the initial patchset.
>
> I saw that as an internal incremental improvement that did not affect
> the API, so I wanted to keep things a bit simpler (as you've requested
> in the past) to get this going, and add that enhancement later.
In general a simple approach is a good way to start when the
problem/use-case is not very well understood; in other words, don't
spend a lot of time/effort optimizing something you don't yet
understand. In this case we know that people want to deploy a *lot*
of containers on a single system so we should design the data
structures appropriately. A list is simply not a good fit here, I
believe/hope you know that too.
> I'll start working on it now. The hash table would simply point to
> lists anyways unless you can recommend a better approach.
I assume when you say "point to lists" you are talking about using
lists for the hash buckets? If so, yes that should be fine at this
point. In general if the per-bucket lists become a bottleneck we can
look at the size of the table (or make it tunable) or even use a
different approach entirely. Ultimately the data store is an
implementation detail private to the audit subsystem in the kernel so
we should be able to change it as necessary without breaking anything.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH ghak90 V6 02/10] audit: add container id
From: Richard Guy Briggs @ 2019-07-16 16:26 UTC (permalink / raw)
To: Paul Moore
Cc: Tycho Andersen, nhorman, linux-api, containers, LKML, dhowells,
Linux-Audit Mailing List, netfilter-devel, ebiederm, simo, netdev,
linux-fsdevel, Eric Paris, Serge E. Hallyn
In-Reply-To: <CAHC9VhTaLqCo8rmAaySJQB+Pf-580=3mvX1rPmtEeb9o5Uy9Qg@mail.gmail.com>
On 2019-07-16 12:08, Paul Moore wrote:
> On Tue, Jul 16, 2019 at 11:37 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2019-07-15 17:09, Paul Moore wrote:
> > > On Mon, Jul 8, 2019 at 2:12 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > On 2019-05-30 19:26, Paul Moore wrote:
> > >
> > > ...
> > >
> > > > > I like the creativity, but I worry that at some point these
> > > > > limitations are going to be raised (limits have a funny way of doing
> > > > > that over time) and we will be in trouble. I say "trouble" because I
> > > > > want to be able to quickly do an audit container ID comparison and
> > > > > we're going to pay a penalty for these larger values (we'll need this
> > > > > when we add multiple auditd support and the requisite record routing).
> > > > >
> > > > > Thinking about this makes me also realize we probably need to think a
> > > > > bit longer about audit container ID conflicts between orchestrators.
> > > > > Right now we just take the value that is given to us by the
> > > > > orchestrator, but if we want to allow multiple container orchestrators
> > > > > to work without some form of cooperation in userspace (I think we have
> > > > > to assume the orchestrators will not talk to each other) we likely
> > > > > need to have some way to block reuse of an audit container ID. We
> > > > > would either need to prevent the orchestrator from explicitly setting
> > > > > an audit container ID to a currently in use value, or instead generate
> > > > > the audit container ID in the kernel upon an event triggered by the
> > > > > orchestrator (e.g. a write to a /proc file). I suspect we should
> > > > > start looking at the idr code, I think we will need to make use of it.
> > > >
> > > > To address this, I'd suggest that it is enforced to only allow the
> > > > setting of descendants and to maintain a master list of audit container
> > > > identifiers (with a hash table if necessary later) that includes the
> > > > container owner.
> > >
> > > We're discussing the audit container ID management policy elsewhere in
> > > this thread so I won't comment on that here, but I did want to say
> > > that we will likely need something better than a simple list of audit
> > > container IDs from the start. It's common for systems to have
> > > thousands of containers now (or multiple thousands), which tells me
> > > that a list is a poor choice. You mentioned a hash table, so I would
> > > suggest starting with that over the list for the initial patchset.
> >
> > I saw that as an internal incremental improvement that did not affect
> > the API, so I wanted to keep things a bit simpler (as you've requested
> > in the past) to get this going, and add that enhancement later.
>
> In general a simple approach is a good way to start when the
> problem/use-case is not very well understood; in other words, don't
> spend a lot of time/effort optimizing something you don't yet
> understand. In this case we know that people want to deploy a *lot*
> of containers on a single system so we should design the data
> structures appropriately. A list is simply not a good fit here, I
> believe/hope you know that too.
Yes, I knew that, which is why I alluded to a hash table...
> > I'll start working on it now. The hash table would simply point to
> > lists anyways unless you can recommend a better approach.
>
> I assume when you say "point to lists" you are talking about using
> lists for the hash buckets? If so, yes that should be fine at this
> point. In general if the per-bucket lists become a bottleneck we can
> look at the size of the table (or make it tunable) or even use a
> different approach entirely. Ultimately the data store is an
> implementation detail private to the audit subsystem in the kernel so
> we should be able to change it as necessary without breaking anything.
Yes, this is what I had in mind. It would be tunable either by a macro
or a config option, so the exact value isn't a critical implementation
detail that can be easily tuned as we gain experience with it. And yes,
the intent was that it was a non-user-perceivable implementation choice
other than performace metrics.
> paul moore
- RGB
^ permalink raw reply
* Re: [PATCH ghak90 V6 02/10] audit: add container id
From: Richard Guy Briggs @ 2019-07-16 19:38 UTC (permalink / raw)
To: Paul Moore
Cc: Tycho Andersen, containers, linux-api, Linux-Audit Mailing List,
linux-fsdevel, LKML, netdev, netfilter-devel, sgrubb, omosnace,
dhowells, simo, Eric Paris, Serge Hallyn, ebiederm, nhorman
In-Reply-To: <CAHC9VhRFeCFSCn=m6wgDK2tXBN1euc2+bw8o=CfNwptk8t=j7A@mail.gmail.com>
On 2019-07-15 16:38, Paul Moore wrote:
> On Mon, Jul 8, 2019 at 1:51 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2019-05-29 11:29, Paul Moore wrote:
>
> ...
>
> > > The idea is that only container orchestrators should be able to
> > > set/modify the audit container ID, and since setting the audit
> > > container ID can have a significant effect on the records captured
> > > (and their routing to multiple daemons when we get there) modifying
> > > the audit container ID is akin to modifying the audit configuration
> > > which is why it is gated by CAP_AUDIT_CONTROL. The current thinking
> > > is that you would only change the audit container ID from one
> > > set/inherited value to another if you were nesting containers, in
> > > which case the nested container orchestrator would need to be granted
> > > CAP_AUDIT_CONTROL (which everyone to date seems to agree is a workable
> > > compromise). We did consider allowing for a chain of nested audit
> > > container IDs, but the implications of doing so are significant
> > > (implementation mess, runtime cost, etc.) so we are leaving that out
> > > of this effort.
> >
> > We had previously discussed the idea of restricting
> > orchestrators/engines from only being able to set the audit container
> > identifier on their own descendants, but it was discarded. I've added a
> > check to ensure this is now enforced.
>
> When we weren't allowing nested orchestrators it wasn't necessary, but
> with the move to support nesting I believe this will be a requirement.
> We might also need/want to restrict audit container ID changes if a
> descendant is acting as a container orchestrator and managing one or
> more audit container IDs; although I'm less certain of the need for
> this.
I was of the opinion it was necessary before with single-layer parallel
orchestrators/engines.
> > I've also added a check to ensure that a process can't set its own audit
> > container identifier ...
>
> What does this protect against, or what problem does this solve?
> Considering how easy it is to fork/exec, it seems like this could be
> trivially bypassed.
Well, for starters, it would remove one layer of nesting. It would
separate the functional layers of processes. Other than that, it seems
like a gut feeling that it is just wrong to allow it. It seems like a
layer violation that one container orchestrator/engine could set its own
audit container identifier and then set its children as well. It would
be its own parent. It would make it harder to verify adherance to
descendancy and inheritance rules.
> > ... and that if the identifier is already set, then the
> > orchestrator/engine must be in a descendant user namespace from the
> > orchestrator that set the previously inherited audit container
> > identifier.
>
> You lost me here ... although I don't like the idea of relying on X
> namespace inheritance for a hard coded policy on setting the audit
> container ID; we've worked hard to keep this independent of any
> definition of a "container" and it would sadden me greatly if we had
> to go back on that.
This would seem to be the one concession I'm reluctantly making to try
to solve this nested container orchestrator/engine challenge.
Would backing off on that descendant user namespace requirement and only
require that a nested audit container identifier only be permitted on a
descendant task be sufficient? It may for this use case, but I suspect
not for additional audit daemons (we're not there yet) and message
routing to those daemons.
The one difference here is that it does not depend on this if the audit
container identifier has not already been set.
> paul moore
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: [PATCH V35 23/29] bpf: Restrict bpf when kernel lockdown is in confidentiality mode
From: Matthew Garrett @ 2019-07-16 20:32 UTC (permalink / raw)
To: Daniel Borkmann
Cc: James Morris, LSM List, Linux Kernel Mailing List, Linux API,
David Howells, Alexei Starovoitov, Network Development,
Chun-Yi Lee
In-Reply-To: <5d363f09-d649-5693-45c0-bb99d69f0f38@iogearbox.net>
On Mon, Jul 15, 2019 at 3:54 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> Hmm, does security_locked_down() ever return a code > 0 or why do you
> have the double check on return code? If not, then for clarity the
> ret code from security_locked_down() should be checked as 'ret < 0'
> as well and out label should be at the memset directly instead.
It doesn't, so I'll update. Thanks!
^ permalink raw reply
* Re: [PATCH V35 15/29] acpi: Ignore acpi_rsdp kernel param when the kernel has been locked down
From: Matthew Garrett @ 2019-07-16 20:34 UTC (permalink / raw)
To: Dave Young
Cc: James Morris, LSM List, Linux Kernel Mailing List, Linux API,
Josh Boyer, David Howells, Borislav Petkov, Kees Cook, linux-acpi
In-Reply-To: <20190716025923.GA5793@dhcp-128-65.nay.redhat.com>
On Mon, Jul 15, 2019 at 7:59 PM Dave Young <dyoung@redhat.com> wrote:
> I'm very sorry I noticed this late, but have to say this will not work for
> X86 with latest kernel code.
No problem, thank you for catching this! I'll update the patch and
send a new version.
^ permalink raw reply
* Re: [PATCH ghak90 V6 02/10] audit: add container id
From: Paul Moore @ 2019-07-16 21:39 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Tycho Andersen, containers, linux-api, Linux-Audit Mailing List,
linux-fsdevel, LKML, netdev, netfilter-devel, sgrubb, omosnace,
dhowells, simo, Eric Paris, Serge Hallyn, ebiederm, nhorman
In-Reply-To: <20190716193828.xvm67iv5jyypvvxp@madcap2.tricolour.ca>
On Tue, Jul 16, 2019 at 3:38 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2019-07-15 16:38, Paul Moore wrote:
> > On Mon, Jul 8, 2019 at 1:51 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > On 2019-05-29 11:29, Paul Moore wrote:
> >
> > ...
> >
> > > > The idea is that only container orchestrators should be able to
> > > > set/modify the audit container ID, and since setting the audit
> > > > container ID can have a significant effect on the records captured
> > > > (and their routing to multiple daemons when we get there) modifying
> > > > the audit container ID is akin to modifying the audit configuration
> > > > which is why it is gated by CAP_AUDIT_CONTROL. The current thinking
> > > > is that you would only change the audit container ID from one
> > > > set/inherited value to another if you were nesting containers, in
> > > > which case the nested container orchestrator would need to be granted
> > > > CAP_AUDIT_CONTROL (which everyone to date seems to agree is a workable
> > > > compromise). We did consider allowing for a chain of nested audit
> > > > container IDs, but the implications of doing so are significant
> > > > (implementation mess, runtime cost, etc.) so we are leaving that out
> > > > of this effort.
> > >
> > > We had previously discussed the idea of restricting
> > > orchestrators/engines from only being able to set the audit container
> > > identifier on their own descendants, but it was discarded. I've added a
> > > check to ensure this is now enforced.
> >
> > When we weren't allowing nested orchestrators it wasn't necessary, but
> > with the move to support nesting I believe this will be a requirement.
> > We might also need/want to restrict audit container ID changes if a
> > descendant is acting as a container orchestrator and managing one or
> > more audit container IDs; although I'm less certain of the need for
> > this.
>
> I was of the opinion it was necessary before with single-layer parallel
> orchestrators/engines.
One of the many things we've disagreed on, but it doesn't really
matter at this point.
> > > I've also added a check to ensure that a process can't set its own audit
> > > container identifier ...
> >
> > What does this protect against, or what problem does this solve?
> > Considering how easy it is to fork/exec, it seems like this could be
> > trivially bypassed.
>
> Well, for starters, it would remove one layer of nesting. It would
> separate the functional layers of processes.
This doesn't seem like something we need to protect against, what's
the harm? My opinion at this point is that we should only add
restrictions to protect against problematic or dangerous situations; I
don't believe one extra layer of nesting counts as either.
Perhaps the container folks on the To/CC line can comment on this? If
there is a valid reason for this restriction, great, let's do it,
otherwise it seems like an unnecessary hard coded policy to me.
> Other than that, it seems
> like a gut feeling that it is just wrong to allow it. It seems like a
> layer violation that one container orchestrator/engine could set its own
> audit container identifier and then set its children as well. It would
> be its own parent.
I suspect you are right that the current crop of container engines
won't do this, but who knows what we'll be doing with "containers" 5,
or even 10, years from now. With that in mind, let me ask the
question again: is allowing an orchestrator the ability to set its own
audit container ID problematic and/or dangerous?
> It would make it harder to verify adherance to descendancy and inheritance rules.
The audit log should contain all the information needed to track that,
right? If it doesn't, then I think we have a problem with the
information we are logging. Right?
> > > ... and that if the identifier is already set, then the
> > > orchestrator/engine must be in a descendant user namespace from the
> > > orchestrator that set the previously inherited audit container
> > > identifier.
> >
> > You lost me here ... although I don't like the idea of relying on X
> > namespace inheritance for a hard coded policy on setting the audit
> > container ID; we've worked hard to keep this independent of any
> > definition of a "container" and it would sadden me greatly if we had
> > to go back on that.
>
> This would seem to be the one concession I'm reluctantly making to try
> to solve this nested container orchestrator/engine challenge.
As I said, you lost me on this - how does this help? A more detailed
explanation of how this helps resolve the nesting problem would be
useful.
> Would backing off on that descendant user namespace requirement and only
> require that a nested audit container identifier only be permitted on a
> descendant task be sufficient? It may for this use case, but I suspect
> not for additional audit daemons (we're not there yet) and message
> routing to those daemons.
>
> The one difference here is that it does not depend on this if the audit
> container identifier has not already been set.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH ghak90 V6 02/10] audit: add container id
From: Richard Guy Briggs @ 2019-07-16 22:03 UTC (permalink / raw)
To: Paul Moore
Cc: Serge E. Hallyn, Tycho Andersen, containers, linux-api,
Linux-Audit Mailing List, linux-fsdevel, LKML, netdev,
netfilter-devel, sgrubb, omosnace, dhowells, simo, Eric Paris,
ebiederm, nhorman
In-Reply-To: <CAHC9VhRTT7JWqNnynvK04wKerjc-3UJ6R1uPtjCAPVr_tW-7MA@mail.gmail.com>
On 2019-07-15 17:04, Paul Moore wrote:
> On Mon, Jul 8, 2019 at 2:06 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2019-05-30 15:29, Paul Moore wrote:
>
> ...
>
> > > [REMINDER: It is an "*audit* container ID" and not a general
> > > "container ID" ;) Smiley aside, I'm not kidding about that part.]
> > >
> > > I'm not interested in supporting/merging something that isn't useful;
> > > if this doesn't work for your use case then we need to figure out what
> > > would work. It sounds like nested containers are much more common in
> > > the lxc world, can you elaborate a bit more on this?
> > >
> > > As far as the possible solutions you mention above, I'm not sure I
> > > like the per-userns audit container IDs, I'd much rather just emit the
> > > necessary tracking information via the audit record stream and let the
> > > log analysis tools figure it out. However, the bigger question is how
> > > to limit (re)setting the audit container ID when you are in a non-init
> > > userns. For reasons already mentioned, using capable() is a non
> > > starter for everything but the initial userns, and using ns_capable()
> > > is equally poor as it essentially allows any userns the ability to
> > > munge it's audit container ID (obviously not good). It appears we
> > > need a different method for controlling access to the audit container
> > > ID.
> >
> > We're not quite ready yet for multiple audit daemons and possibly not
> > yet for audit namespaces, but this is starting to look a lot like the
> > latter.
>
> A few quick comments on audit namespaces: the audit container ID is
> not envisioned as a new namespace (even in nested form) and neither do
> I consider running multiple audit daemons to be a new namespace.
I can picture either one.
> From my perspective we create namespaces to allow us to redefine a
> global resource for some subset of the system, e.g. providing a unique
> /tmp for some number of processes on the system. While it may be
> tempting to think of the audit container ID as something we could
> "namespace", especially when multiple audit daemons are concerned, in
> some ways this would be counter productive; the audit container ID is
> intended to be a global ID that can be used to associate audit event
> records with a "container" where the "container" is defined by an
> orchestrator outside the audit subsystem. The global nature of the
> audit container ID allows us to maintain a sane(ish) view of the
> system in the audit log, if we were to "namespace" the audit container
> ID such that the value was no longer guaranteed to be unique
> throughout the system, we would need to additionally track the audit
> namespace along with the audit container ID which starts to border on
> insanity IMHO.
Understood. And mostly agree. Any audit namespace would have to be a
hybrid anyways, since only the init one would have full access to audit
resources. All the others would be somewhat neutered. And in the case
of checking for previous usage of a contid, if it was not already in use
in the hypothetical audit namespace but was in use elsewhere in the
system and we blocked its usage in this namespace, it would leak that
information by blocking it.
I saw it as a way of permitting layering with the natural descendancy
structure showing that hierarchy. The potential flaw with my reasoning
is that a parent could exit and its children would get re-parented onto
its next ancestor, so the intermediate task with an intermediate contid
would break that contid documentation chain.
> > If we can't trust ns_capable() then why are we passing on
> > CAP_AUDIT_CONTROL? It is being passed down and not stripped purposely
> > by the orchestrator/engine. If ns_capable() isn't inherited how is it
> > gained otherwise? Can it be inserted by cotainer image? I think the
> > answer is "no". Either we trust ns_capable() or we have audit
> > namespaces (recommend based on user namespace) (or both).
>
> My thinking is that since ns_capable() checks the credentials with
> respect to the current user namespace we can't rely on it to control
> access since it would be possible for a privileged process running
> inside an unprivileged container to manipulate the audit container ID
> (containerized process has CAP_AUDIT_CONTROL, e.g. running as root in
> the container, while the container itself does not).
What makes an unprivileged container unprivileged? "root", or "CAP_*"?
If CAP_AUDIT_CONTROL is granted, does "root" matter? Does it matter
what user namespace it is in? I understand that root is *gained* in an
unprivileged user namespace, but capabilities are inherited or permitted
and that process either has it or it doesn't and an unprivileged user
namespace can't gain a capability that has been rescinded. Different
subsystems use the userid or capabilities or both to determine
privileges. In this case, is the userid relevant?
> > At this point I would say we are at an impasse unless we trust
> > ns_capable() or we implement audit namespaces.
>
> I'm not sure how we can trust ns_capable(), but if you can think of a
> way I would love to hear it. I'm also not sure how namespacing audit
> is helpful (see my above comments), but if you think it is please
> explain.
So if we are not namespacing, why do we not trust capabilities?
> > I don't think another mechanism to trust nested orchestrators/engines
> > will buy us anything.
> >
> > Am I missing something?
>
> Based on your questions/comments above it looks like your
> understanding of ns_capable() does not match mine; if I'm thinking
> about ns_capable() incorrectly, please educate me.
>
> --
> paul moore
> www.paul-moore.com
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: [PATCH V35 27/29] tracefs: Restrict tracefs when the kernel is locked down
From: Steven Rostedt @ 2019-07-16 23:14 UTC (permalink / raw)
To: Matthew Garrett
Cc: jmorris, linux-security-module, linux-kernel, linux-api,
Matthew Garrett
In-Reply-To: <20190715195946.223443-28-matthewgarrett@google.com>
On Mon, 15 Jul 2019 12:59:44 -0700
Matthew Garrett <matthewgarrett@google.com> wrote:
> Tracefs may release more information about the kernel than desirable, so
> restrict it when the kernel is locked down in confidentiality mode by
> preventing open().
>
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> ---
>
> @@ -389,6 +414,7 @@ struct dentry *tracefs_create_file(const char *name, umode_t mode,
> {
> struct dentry *dentry;
> struct inode *inode;
> + struct file_operations *proxy_fops;
Small nit, but please add this as the first declaration, to keep the
"upside-down x-mas tree" look. I know some of the other functions in
this file don't follow that (which should be cleaned up some day), but
I'd like to avoid adding more that breaks the aesthetic of the code.
>
> if (!(mode & S_IFMT))
> mode |= S_IFREG;
> @@ -402,8 +428,18 @@ struct dentry *tracefs_create_file(const char *name, umode_t mode,
> if (unlikely(!inode))
> return failed_creating(dentry);
>
> + proxy_fops = kzalloc(sizeof(struct file_operations), GFP_KERNEL);
> + if (!proxy_fops)
> + return failed_creating(dentry);
> +
> + if (fops)
I think you meant "if (!fops)".
-- Steve
> + fops = &tracefs_file_operations;
> +
> + dentry->d_fsdata = (void *)fops;
> + memcpy(proxy_fops, fops, sizeof(*proxy_fops));
> + proxy_fops->open = default_open_file;
> inode->i_mode = mode;
> - inode->i_fop = fops ? fops : &tracefs_file_operations;
> + inode->i_fop = proxy_fops;
> inode->i_private = data;
> d_instantiate(dentry, inode);
> fsnotify_create(dentry->d_parent->d_inode, dentry);
^ permalink raw reply
* Re: [PATCH V35 27/29] tracefs: Restrict tracefs when the kernel is locked down
From: Matthew Garrett @ 2019-07-16 23:22 UTC (permalink / raw)
To: Steven Rostedt
Cc: James Morris, LSM List, Linux Kernel Mailing List, Linux API
In-Reply-To: <20190716191439.59a1ac32@gandalf.local.home>
On Tue, Jul 16, 2019 at 4:14 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> Small nit, but please add this as the first declaration, to keep the
> "upside-down x-mas tree" look. I know some of the other functions in
> this file don't follow that (which should be cleaned up some day), but
> I'd like to avoid adding more that breaks the aesthetic of the code.
ACK.
> > +
> > + if (fops)
>
> I think you meant "if (!fops)".
Blink. Whoops! Yup.
^ permalink raw reply
* Re: [PATCH ghak90 V6 02/10] audit: add container id
From: Paul Moore @ 2019-07-16 23:30 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Serge E. Hallyn, Tycho Andersen, containers, linux-api,
Linux-Audit Mailing List, linux-fsdevel, LKML, netdev,
netfilter-devel, sgrubb, omosnace, dhowells, simo, Eric Paris,
ebiederm, nhorman
In-Reply-To: <20190716220320.sotbfqplgdructg7@madcap2.tricolour.ca>
On Tue, Jul 16, 2019 at 6:03 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2019-07-15 17:04, Paul Moore wrote:
> > On Mon, Jul 8, 2019 at 2:06 PM Richard Guy Briggs <rgb@redhat.com> wrote:
...
> > > If we can't trust ns_capable() then why are we passing on
> > > CAP_AUDIT_CONTROL? It is being passed down and not stripped purposely
> > > by the orchestrator/engine. If ns_capable() isn't inherited how is it
> > > gained otherwise? Can it be inserted by cotainer image? I think the
> > > answer is "no". Either we trust ns_capable() or we have audit
> > > namespaces (recommend based on user namespace) (or both).
> >
> > My thinking is that since ns_capable() checks the credentials with
> > respect to the current user namespace we can't rely on it to control
> > access since it would be possible for a privileged process running
> > inside an unprivileged container to manipulate the audit container ID
> > (containerized process has CAP_AUDIT_CONTROL, e.g. running as root in
> > the container, while the container itself does not).
>
> What makes an unprivileged container unprivileged? "root", or "CAP_*"?
My understanding is that when most people refer to an unprivileged
container they are referring to a container run without capabilities
or a container run by a user other than root. I'm sure there are
better definitions out there, by folks much smarter than me on these
things, but that's my working definition.
> If CAP_AUDIT_CONTROL is granted, does "root" matter?
Our discussions here have been about capabilities, not UIDs. The only
reason root might matter is that it generally has the full capability
set.
> Does it matter what user namespace it is in?
What likely matters is what check is called: capable() or
ns_capable(). Those can yield very different results.
> I understand that root is *gained* in an
> unprivileged user namespace, but capabilities are inherited or permitted
> and that process either has it or it doesn't and an unprivileged user
> namespace can't gain a capability that has been rescinded. Different
> subsystems use the userid or capabilities or both to determine
> privileges.
Once again, I believe the important thing to focus on here is
capable() vs ns_capable(). We can't safely rely on ns_capable() for
the audit container ID policy since that is easily met inside the
container regardless of the process' creds which started the
container.
> In this case, is the userid relevant?
We don't do UID checks, we do capability checks, so yes, the UID is irrelevant.
> > > At this point I would say we are at an impasse unless we trust
> > > ns_capable() or we implement audit namespaces.
> >
> > I'm not sure how we can trust ns_capable(), but if you can think of a
> > way I would love to hear it. I'm also not sure how namespacing audit
> > is helpful (see my above comments), but if you think it is please
> > explain.
>
> So if we are not namespacing, why do we not trust capabilities?
We can trust capable(CAP_AUDIT_CONTROL) for enforcing audit container
ID policy, we can not trust ns_capable(CAP_AUDIT_CONTROL).
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Add a new fchmodat4() syscall, v2
From: Palmer Dabbelt @ 2019-07-17 1:27 UTC (permalink / raw)
To: viro, linux-kernel, linux-fsdevel, linux-api
Cc: rth, ink, mattst88, linux, catalin.marinas, will, tony.luck,
fenghua.yu, geert, monstr, ralf, paul.burton, jhogan,
James.Bottomley, deller, benh, paulus, mpe, heiko.carstens, gor,
borntraeger, ysato, dalias, davem, luto, tglx, mingo, bp, hpa,
x86, Arnd Bergmann, peterz, acme, alexander.shishkin, jolsa,
namhyung, Palmer Dabbelt <palm>
This patch set adds fchmodat4(), a new syscall. The actual
implementation is super simple: essentially it's just the same as
fchmodat(), but LOOKUP_FOLLOW is conditionally set based on the flags.
I've attempted to make this match "man 2 fchmodat" as closely as
possible, which says EINVAL is returned for invalid flags (as opposed to
ENOTSUPP, which is currently returned by glibc for AT_SYMLINK_NOFOLLOW).
I have a sketch of a glibc patch that I haven't even compiled yet, but
seems fairly straight-forward:
diff --git a/sysdeps/unix/sysv/linux/fchmodat.c b/sysdeps/unix/sysv/linux/fchmodat.c
index 6d9cbc1ce9e0..b1beab76d56c 100644
--- a/sysdeps/unix/sysv/linux/fchmodat.c
+++ b/sysdeps/unix/sysv/linux/fchmodat.c
@@ -29,12 +29,36 @@
int
fchmodat (int fd, const char *file, mode_t mode, int flag)
{
- if (flag & ~AT_SYMLINK_NOFOLLOW)
- return INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL);
-#ifndef __NR_lchmod /* Linux so far has no lchmod syscall. */
+ /* There are four paths through this code:
+ - The flags are zero. In this case it's fine to call fchmodat.
+ - The flags are non-zero and glibc doesn't have access to
+ __NR_fchmodat4. In this case all we can do is emulate the error codes
+ defined by the glibc interface from userspace.
+ - The flags are non-zero, glibc has __NR_fchmodat4, and the kernel has
+ fchmodat4. This is the simplest case, as the fchmodat4 syscall exactly
+ matches glibc's library interface so it can be called directly.
+ - The flags are non-zero, glibc has __NR_fchmodat4, but the kernel does
+ not. In this case we must respect the error codes defined by the glibc
+ interface instead of returning ENOSYS.
+ The intent here is to ensure that the kernel is called at most once per
+ library call, and that the error types defined by glibc are always
+ respected. */
+
+#ifdef __NR_fchmodat4
+ long result;
+#endif
+
+ if (flag == 0)
+ return INLINE_SYSCALL (fchmodat, 3, fd, file, mode);
+
+#ifdef __NR_fchmodat4
+ result = INLINE_SYSCALL (fchmodat4, 4, fd, file, mode, flag);
+ if (result == 0 || errno != ENOSYS)
+ return result;
+#endif
+
if (flag & AT_SYMLINK_NOFOLLOW)
return INLINE_SYSCALL_ERROR_RETURN_VALUE (ENOTSUP);
-#endif
- return INLINE_SYSCALL (fchmodat, 3, fd, file, mode);
+ return INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL);
}
I've never added a new syscall before so I'm not really sure what the
proper procedure to follow is. Based on the feedback from my v1 patch
set it seems this is somewhat uncontroversial. At this point I don't
think there's anything I'm missing, though note that I haven't gotten
around to testing it this time because the diff from v1 is trivial for
any platform I could reasonably test on. The v1 patches suggest a
simple test case, but I didn't re-run it because I don't want to reboot
my laptop.
$ touch test-file
$ ln -s test-file test-link
$ cat > test.c
#include <fcntl.h>
#include <stdio.h>
#include <unistd.h>
int main(int argc, char **argv)
{
long out;
out = syscall(434, AT_FDCWD, "test-file", 0x888, AT_SYMLINK_NOFOLLOW);
printf("fchmodat4(AT_FDCWD, \"test-file\", 0x888, AT_SYMLINK_NOFOLLOW): %ld\n", out);
out = syscall(434, AT_FDCWD, "test-file", 0x888, 0);
printf("fchmodat4(AT_FDCWD, \"test-file\", 0x888, 0): %ld\n", out);
out = syscall(268, AT_FDCWD, "test-file", 0x888);
printf("fchmodat(AT_FDCWD, \"test-file\", 0x888): %ld\n", out);
out = syscall(434, AT_FDCWD, "test-link", 0x888, AT_SYMLINK_NOFOLLOW);
printf("fchmodat4(AT_FDCWD, \"test-link\", 0x888, AT_SYMLINK_NOFOLLOW): %ld\n", out);
out = syscall(434, AT_FDCWD, "test-link", 0x888, 0);
printf("fchmodat4(AT_FDCWD, \"test-link\", 0x888, 0): %ld\n", out);
out = syscall(268, AT_FDCWD, "test-link", 0x888);
printf("fchmodat(AT_FDCWD, \"test-link\", 0x888): %ld\n", out);
return 0;
}
$ gcc test.c -o test
$ ./test
fchmodat4(AT_FDCWD, "test-file", 0x888, AT_SYMLINK_NOFOLLOW): 0
fchmodat4(AT_FDCWD, "test-file", 0x888, 0): 0
fchmodat(AT_FDCWD, "test-file", 0x888): 0
fchmodat4(AT_FDCWD, "test-link", 0x888, AT_SYMLINK_NOFOLLOW): -1
fchmodat4(AT_FDCWD, "test-link", 0x888, 0): 0
fchmodat(AT_FDCWD, "test-link", 0x888): 0
I've only built this on 64-bit x86.
Changes since v1 [20190531191204.4044-1-palmer@sifive.com]:
* All architectures are now supported, which support squashed into a
single patch.
* The do_fchmodat() helper function has been removed, in favor of directly
calling do_fchmodat4().
* The patches are based on 5.2 instead of 5.1.
^ permalink raw reply
* [PATCH v2 1/4] Non-functional cleanup of a "__user * filename"
From: Palmer Dabbelt @ 2019-07-17 1:27 UTC (permalink / raw)
To: viro, linux-kernel, linux-fsdevel, linux-api
Cc: rth, ink, mattst88, linux, catalin.marinas, will, tony.luck,
fenghua.yu, geert, monstr, ralf, paul.burton, jhogan,
James.Bottomley, deller, benh, paulus, mpe, heiko.carstens, gor,
borntraeger, ysato, dalias, davem, luto, tglx, mingo, bp, hpa,
x86, Arnd Bergmann, peterz, acme, alexander.shishkin, jolsa,
namhyung, Palmer Dabbelt <palm>
In-Reply-To: <20190717012719.5524-1-palmer@sifive.com>
The next patch defines a very similar interface, which I copied from
this definition. Since I'm touching it anyway I don't see any reason
not to just go fix this one up.
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
include/linux/syscalls.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 2bcef4c70183..e1c20f1d0525 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -431,7 +431,7 @@ asmlinkage long sys_chdir(const char __user *filename);
asmlinkage long sys_fchdir(unsigned int fd);
asmlinkage long sys_chroot(const char __user *filename);
asmlinkage long sys_fchmod(unsigned int fd, umode_t mode);
-asmlinkage long sys_fchmodat(int dfd, const char __user * filename,
+asmlinkage long sys_fchmodat(int dfd, const char __user *filename,
umode_t mode);
asmlinkage long sys_fchownat(int dfd, const char __user *filename, uid_t user,
gid_t group, int flag);
--
2.21.0
^ permalink raw reply related
* [PATCH v2 2/4] Add fchmodat4(), a new syscall
From: Palmer Dabbelt @ 2019-07-17 1:27 UTC (permalink / raw)
To: viro, linux-kernel, linux-fsdevel, linux-api
Cc: rth, ink, mattst88, linux, catalin.marinas, will, tony.luck,
fenghua.yu, geert, monstr, ralf, paul.burton, jhogan,
James.Bottomley, deller, benh, paulus, mpe, heiko.carstens, gor,
borntraeger, ysato, dalias, davem, luto, tglx, mingo, bp, hpa,
x86, Arnd Bergmann, peterz, acme, alexander.shishkin, jolsa,
namhyung, Palmer Dabbelt <palm>
In-Reply-To: <20190717012719.5524-1-palmer@sifive.com>
man 3p says that fchmodat() takes a flags argument, but the Linux
syscall does not. There doesn't appear to be a good userspace
workaround for this issue but the implementation in the kernel is pretty
straight-forward. The specific use case where the missing flags came up
was WRT a fuse filesystem implemenation, but the functionality is pretty
generic so I'm assuming there would be other use cases.
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
fs/open.c | 20 ++++++++++++++++----
include/linux/syscalls.h | 7 +++++--
2 files changed, 21 insertions(+), 6 deletions(-)
diff --git a/fs/open.c b/fs/open.c
index b5b80469b93d..2f72b4d6a2c1 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -569,11 +569,17 @@ SYSCALL_DEFINE2(fchmod, unsigned int, fd, umode_t, mode)
return ksys_fchmod(fd, mode);
}
-int do_fchmodat(int dfd, const char __user *filename, umode_t mode)
+int do_fchmodat4(int dfd, const char __user *filename, umode_t mode, int flags)
{
struct path path;
int error;
- unsigned int lookup_flags = LOOKUP_FOLLOW;
+ unsigned int lookup_flags;
+
+ if (unlikely(flags & ~AT_SYMLINK_NOFOLLOW))
+ return -EINVAL;
+
+ lookup_flags = flags & AT_SYMLINK_NOFOLLOW ? 0 : LOOKUP_FOLLOW;
+
retry:
error = user_path_at(dfd, filename, lookup_flags, &path);
if (!error) {
@@ -587,15 +593,21 @@ int do_fchmodat(int dfd, const char __user *filename, umode_t mode)
return error;
}
+SYSCALL_DEFINE4(fchmodat4, int, dfd, const char __user *, filename,
+ umode_t, mode, int, flags)
+{
+ return do_fchmodat4(dfd, filename, mode, flags);
+}
+
SYSCALL_DEFINE3(fchmodat, int, dfd, const char __user *, filename,
umode_t, mode)
{
- return do_fchmodat(dfd, filename, mode);
+ return do_fchmodat4(dfd, filename, mode, 0);
}
SYSCALL_DEFINE2(chmod, const char __user *, filename, umode_t, mode)
{
- return do_fchmodat(AT_FDCWD, filename, mode);
+ return do_fchmodat4(AT_FDCWD, filename, mode, 0);
}
static int chown_common(const struct path *path, uid_t user, gid_t group)
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index e1c20f1d0525..a4bde25ad264 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -433,6 +433,8 @@ asmlinkage long sys_chroot(const char __user *filename);
asmlinkage long sys_fchmod(unsigned int fd, umode_t mode);
asmlinkage long sys_fchmodat(int dfd, const char __user *filename,
umode_t mode);
+asmlinkage long sys_fchmodat4(int dfd, const char __user *filename,
+ umode_t mode, int flags);
asmlinkage long sys_fchownat(int dfd, const char __user *filename, uid_t user,
gid_t group, int flag);
asmlinkage long sys_fchown(unsigned int fd, uid_t user, gid_t group);
@@ -1320,11 +1322,12 @@ static inline long ksys_link(const char __user *oldname,
return do_linkat(AT_FDCWD, oldname, AT_FDCWD, newname, 0);
}
-extern int do_fchmodat(int dfd, const char __user *filename, umode_t mode);
+extern int do_fchmodat4(int dfd, const char __user *filename, umode_t mode,
+ int flags);
static inline int ksys_chmod(const char __user *filename, umode_t mode)
{
- return do_fchmodat(AT_FDCWD, filename, mode);
+ return do_fchmodat4(AT_FDCWD, filename, mode, 0);
}
extern long do_faccessat(int dfd, const char __user *filename, int mode);
--
2.21.0
^ permalink raw reply related
* [PATCH v2 3/4] arch: Register fchmodat4, usually as syscall 434
From: Palmer Dabbelt @ 2019-07-17 1:27 UTC (permalink / raw)
To: viro, linux-kernel, linux-fsdevel, linux-api
Cc: rth, ink, mattst88, linux, catalin.marinas, will, tony.luck,
fenghua.yu, geert, monstr, ralf, paul.burton, jhogan,
James.Bottomley, deller, benh, paulus, mpe, heiko.carstens, gor,
borntraeger, ysato, dalias, davem, luto, tglx, mingo, bp, hpa,
x86, Arnd Bergmann, peterz, acme, alexander.shishkin, jolsa,
namhyung, Palmer Dabbelt <palm>
In-Reply-To: <20190717012719.5524-1-palmer@sifive.com>
This registers the new fchmodat4 syscall in most places as nuber 434,
with alpha being the exception where it's 544. I found all these sites
by grepping for fspick, which I assume has found me everything.
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
arch/alpha/kernel/syscalls/syscall.tbl | 1 +
arch/arm/tools/syscall.tbl | 1 +
arch/arm64/include/asm/unistd32.h | 2 ++
arch/ia64/kernel/syscalls/syscall.tbl | 1 +
arch/m68k/kernel/syscalls/syscall.tbl | 1 +
arch/microblaze/kernel/syscalls/syscall.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n32.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n64.tbl | 1 +
arch/mips/kernel/syscalls/syscall_o32.tbl | 1 +
arch/parisc/kernel/syscalls/syscall.tbl | 1 +
arch/powerpc/kernel/syscalls/syscall.tbl | 1 +
arch/s390/kernel/syscalls/syscall.tbl | 1 +
arch/sh/kernel/syscalls/syscall.tbl | 1 +
arch/sparc/kernel/syscalls/syscall.tbl | 1 +
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
include/uapi/asm-generic/unistd.h | 5 ++++-
17 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/arch/alpha/kernel/syscalls/syscall.tbl b/arch/alpha/kernel/syscalls/syscall.tbl
index 9e7704e44f6d..6c4ef43c8b52 100644
--- a/arch/alpha/kernel/syscalls/syscall.tbl
+++ b/arch/alpha/kernel/syscalls/syscall.tbl
@@ -473,3 +473,4 @@
541 common fsconfig sys_fsconfig
542 common fsmount sys_fsmount
543 common fspick sys_fspick
+544 common fcmodat4 sys_fchmodat4
diff --git a/arch/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl
index aaf479a9e92d..c008b76fbf92 100644
--- a/arch/arm/tools/syscall.tbl
+++ b/arch/arm/tools/syscall.tbl
@@ -447,3 +447,4 @@
431 common fsconfig sys_fsconfig
432 common fsmount sys_fsmount
433 common fspick sys_fspick
+434 common fchmodat4 sys_fchmodat4
diff --git a/arch/arm64/include/asm/unistd32.h b/arch/arm64/include/asm/unistd32.h
index aa995920bd34..049471b468c1 100644
--- a/arch/arm64/include/asm/unistd32.h
+++ b/arch/arm64/include/asm/unistd32.h
@@ -875,6 +875,8 @@ __SYSCALL(__NR_fsconfig, sys_fsconfig)
__SYSCALL(__NR_fsmount, sys_fsmount)
#define __NR_fspick 433
__SYSCALL(__NR_fspick, sys_fspick)
+#define __NR_fchmodat4 434
+__SYSCALL(__NR_fchmodat4, sys_fchmodat4)
/*
* Please add new compat syscalls above this comment and update
diff --git a/arch/ia64/kernel/syscalls/syscall.tbl b/arch/ia64/kernel/syscalls/syscall.tbl
index e01df3f2f80d..d16e9801fe82 100644
--- a/arch/ia64/kernel/syscalls/syscall.tbl
+++ b/arch/ia64/kernel/syscalls/syscall.tbl
@@ -354,3 +354,4 @@
431 common fsconfig sys_fsconfig
432 common fsmount sys_fsmount
433 common fspick sys_fspick
+434 common fchmodat4 sys_fchmodat4
diff --git a/arch/m68k/kernel/syscalls/syscall.tbl b/arch/m68k/kernel/syscalls/syscall.tbl
index 7e3d0734b2f3..1bbff1a9153c 100644
--- a/arch/m68k/kernel/syscalls/syscall.tbl
+++ b/arch/m68k/kernel/syscalls/syscall.tbl
@@ -433,3 +433,4 @@
431 common fsconfig sys_fsconfig
432 common fsmount sys_fsmount
433 common fspick sys_fspick
+434 common fchmodat4 sys_fchmodat4
diff --git a/arch/microblaze/kernel/syscalls/syscall.tbl b/arch/microblaze/kernel/syscalls/syscall.tbl
index 26339e417695..3ed878cb10a3 100644
--- a/arch/microblaze/kernel/syscalls/syscall.tbl
+++ b/arch/microblaze/kernel/syscalls/syscall.tbl
@@ -439,3 +439,4 @@
431 common fsconfig sys_fsconfig
432 common fsmount sys_fsmount
433 common fspick sys_fspick
+434 common fchmodat4 sys_fchmodat4
diff --git a/arch/mips/kernel/syscalls/syscall_n32.tbl b/arch/mips/kernel/syscalls/syscall_n32.tbl
index 0e2dd68ade57..916cdb808e62 100644
--- a/arch/mips/kernel/syscalls/syscall_n32.tbl
+++ b/arch/mips/kernel/syscalls/syscall_n32.tbl
@@ -372,3 +372,4 @@
431 n32 fsconfig sys_fsconfig
432 n32 fsmount sys_fsmount
433 n32 fspick sys_fspick
+434 n32 fchmodat4 sys_fchmodat4
diff --git a/arch/mips/kernel/syscalls/syscall_n64.tbl b/arch/mips/kernel/syscalls/syscall_n64.tbl
index 5eebfa0d155c..48b4badb1914 100644
--- a/arch/mips/kernel/syscalls/syscall_n64.tbl
+++ b/arch/mips/kernel/syscalls/syscall_n64.tbl
@@ -348,3 +348,4 @@
431 n64 fsconfig sys_fsconfig
432 n64 fsmount sys_fsmount
433 n64 fspick sys_fspick
+434 n64 fchmodat4 sys_fchmodat4
diff --git a/arch/mips/kernel/syscalls/syscall_o32.tbl b/arch/mips/kernel/syscalls/syscall_o32.tbl
index 3cc1374e02d0..0a2ec339882a 100644
--- a/arch/mips/kernel/syscalls/syscall_o32.tbl
+++ b/arch/mips/kernel/syscalls/syscall_o32.tbl
@@ -421,3 +421,4 @@
431 o32 fsconfig sys_fsconfig
432 o32 fsmount sys_fsmount
433 o32 fspick sys_fspick
+434 o32 fchmodat4 sys_fchmodat4
diff --git a/arch/parisc/kernel/syscalls/syscall.tbl b/arch/parisc/kernel/syscalls/syscall.tbl
index c9e377d59232..07d38477f8f7 100644
--- a/arch/parisc/kernel/syscalls/syscall.tbl
+++ b/arch/parisc/kernel/syscalls/syscall.tbl
@@ -430,3 +430,4 @@
431 common fsconfig sys_fsconfig
432 common fsmount sys_fsmount
433 common fspick sys_fspick
+434 common fchmodat4 sys_fchmodat4
diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
index 103655d84b4b..3542b33d1bf3 100644
--- a/arch/powerpc/kernel/syscalls/syscall.tbl
+++ b/arch/powerpc/kernel/syscalls/syscall.tbl
@@ -515,3 +515,4 @@
431 common fsconfig sys_fsconfig
432 common fsmount sys_fsmount
433 common fspick sys_fspick
+434 common fchmodat4 sys_fchmodat4
diff --git a/arch/s390/kernel/syscalls/syscall.tbl b/arch/s390/kernel/syscalls/syscall.tbl
index e822b2964a83..cf7b3086ffaa 100644
--- a/arch/s390/kernel/syscalls/syscall.tbl
+++ b/arch/s390/kernel/syscalls/syscall.tbl
@@ -436,3 +436,4 @@
431 common fsconfig sys_fsconfig sys_fsconfig
432 common fsmount sys_fsmount sys_fsmount
433 common fspick sys_fspick sys_fspick
+434 common fchmodat4 sys_fchmodat4 sys_fchmodat4
diff --git a/arch/sh/kernel/syscalls/syscall.tbl b/arch/sh/kernel/syscalls/syscall.tbl
index 016a727d4357..5afa9fb9b4d3 100644
--- a/arch/sh/kernel/syscalls/syscall.tbl
+++ b/arch/sh/kernel/syscalls/syscall.tbl
@@ -436,3 +436,4 @@
431 common fsconfig sys_fsconfig
432 common fsmount sys_fsmount
433 common fspick sys_fspick
+434 common fchmodat4 sys_fchmodat4
diff --git a/arch/sparc/kernel/syscalls/syscall.tbl b/arch/sparc/kernel/syscalls/syscall.tbl
index e047480b1605..ff19eb8185f3 100644
--- a/arch/sparc/kernel/syscalls/syscall.tbl
+++ b/arch/sparc/kernel/syscalls/syscall.tbl
@@ -479,3 +479,4 @@
431 common fsconfig sys_fsconfig
432 common fsmount sys_fsmount
433 common fspick sys_fspick
+434 common fchmodat4 sys_fchmodat4
diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
index ad968b7bac72..39609fa3536d 100644
--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl
@@ -438,3 +438,4 @@
431 i386 fsconfig sys_fsconfig __ia32_sys_fsconfig
432 i386 fsmount sys_fsmount __ia32_sys_fsmount
433 i386 fspick sys_fspick __ia32_sys_fspick
+434 i386 fchmodat4 sys_fchmodat4 __ia32_sys_fchmodat4
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index b4e6f9e6204a..b92d5b195e66 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -355,6 +355,7 @@
431 common fsconfig __x64_sys_fsconfig
432 common fsmount __x64_sys_fsmount
433 common fspick __x64_sys_fspick
+434 common fchmodat4 __x64_sys_fchmodat4
#
# x32-specific system call numbers start at 512 to avoid cache impact
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index a87904daf103..6a7ba1e69e42 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -845,8 +845,11 @@ __SYSCALL(__NR_fsmount, sys_fsmount)
#define __NR_fspick 433
__SYSCALL(__NR_fspick, sys_fspick)
+#define __NR_fchmodat4 434
+__SYSCALL(__NR_fchmodat4, sys_fchmodat4)
+
#undef __NR_syscalls
-#define __NR_syscalls 434
+#define __NR_syscalls 435
/*
* 32 bit systems traditionally used different
--
2.21.0
^ permalink raw reply related
* [PATCH v2 4/4] tools: Add fchmodat4
From: Palmer Dabbelt @ 2019-07-17 1:27 UTC (permalink / raw)
To: viro, linux-kernel, linux-fsdevel, linux-api
Cc: rth, ink, mattst88, linux, catalin.marinas, will, tony.luck,
fenghua.yu, geert, monstr, ralf, paul.burton, jhogan,
James.Bottomley, deller, benh, paulus, mpe, heiko.carstens, gor,
borntraeger, ysato, dalias, davem, luto, tglx, mingo, bp, hpa,
x86, Arnd Bergmann, peterz, acme, alexander.shishkin, jolsa,
namhyung, Palmer Dabbelt <palm>
In-Reply-To: <20190717012719.5524-1-palmer@sifive.com>
I'm not sure why it's necessary to add this explicitly to tools/ as well
as arch/, but there were a few instances of fspick in here so I blindly
added fchmodat4 in the same fashion.
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
tools/include/uapi/asm-generic/unistd.h | 4 +++-
tools/perf/arch/x86/entry/syscalls/syscall_64.tbl | 1 +
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/tools/include/uapi/asm-generic/unistd.h b/tools/include/uapi/asm-generic/unistd.h
index a87904daf103..36232ea94956 100644
--- a/tools/include/uapi/asm-generic/unistd.h
+++ b/tools/include/uapi/asm-generic/unistd.h
@@ -844,9 +844,11 @@ __SYSCALL(__NR_fsconfig, sys_fsconfig)
__SYSCALL(__NR_fsmount, sys_fsmount)
#define __NR_fspick 433
__SYSCALL(__NR_fspick, sys_fspick)
+#define __NR_fchmodat4 434
+__SYSCALL(__NR_fchmodat4, sys_fchmodat4)
#undef __NR_syscalls
-#define __NR_syscalls 434
+#define __NR_syscalls 435
/*
* 32 bit systems traditionally used different
diff --git a/tools/perf/arch/x86/entry/syscalls/syscall_64.tbl b/tools/perf/arch/x86/entry/syscalls/syscall_64.tbl
index b4e6f9e6204a..b92d5b195e66 100644
--- a/tools/perf/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/tools/perf/arch/x86/entry/syscalls/syscall_64.tbl
@@ -355,6 +355,7 @@
431 common fsconfig __x64_sys_fsconfig
432 common fsmount __x64_sys_fsmount
433 common fspick __x64_sys_fspick
+434 common fchmodat4 __x64_sys_fchmodat4
#
# x32-specific system call numbers start at 512 to avoid cache impact
--
2.21.0
^ permalink raw reply related
* Re: [PATCH v2 2/4] Add fchmodat4(), a new syscall
From: Al Viro @ 2019-07-17 1:48 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: linux-kernel, linux-fsdevel, linux-api, Arnd Bergmann, rth, ink,
mattst88, linux, catalin.marinas, will, tony.luck, fenghua.yu,
geert, monstr, ralf, paul.burton, jhogan, James.Bottomley, deller,
benh, paulus, mpe, heiko.carstens, gor, borntraeger, ysato,
dalias, davem, luto, tglx, mingo, bp, hpa, x86, peterz, acme,
alexand
In-Reply-To: <20190717012719.5524-3-palmer@sifive.com>
On Tue, Jul 16, 2019 at 06:27:17PM -0700, Palmer Dabbelt wrote:
> -int do_fchmodat(int dfd, const char __user *filename, umode_t mode)
> +int do_fchmodat4(int dfd, const char __user *filename, umode_t mode, int flags)
> {
> struct path path;
> int error;
> - unsigned int lookup_flags = LOOKUP_FOLLOW;
> + unsigned int lookup_flags;
> +
> + if (unlikely(flags & ~AT_SYMLINK_NOFOLLOW))
> + return -EINVAL;
> +
> + lookup_flags = flags & AT_SYMLINK_NOFOLLOW ? 0 : LOOKUP_FOLLOW;
> +
Why not do that in sys_fchmodat4() itself, passing lookup_flags to
do_fchmodat() and updating old callers to pass it 0 as extra argument?
^ permalink raw reply
* Re: [PATCH v2 2/4] Add fchmodat4(), a new syscall
From: Palmer Dabbelt @ 2019-07-17 2:12 UTC (permalink / raw)
To: viro
Cc: linux-kernel, linux-fsdevel, linux-api, Arnd Bergmann, rth, ink,
mattst88, linux, catalin.marinas, will, tony.luck, fenghua.yu,
geert, monstr, ralf, paul.burton, jhogan, James.Bottomley, deller,
benh, paulus, mpe, heiko.carstens, gor, borntraeger, ysato,
dalias, davem, luto, tglx, mingo, bp, hpa, x86, peterz, acme,
alexand
In-Reply-To: <20190717014802.GS17978@ZenIV.linux.org.uk>
On Tue, 16 Jul 2019 18:48:02 PDT (-0700), viro@zeniv.linux.org.uk wrote:
> On Tue, Jul 16, 2019 at 06:27:17PM -0700, Palmer Dabbelt wrote:
>
>> -int do_fchmodat(int dfd, const char __user *filename, umode_t mode)
>> +int do_fchmodat4(int dfd, const char __user *filename, umode_t mode, int flags)
>> {
>> struct path path;
>> int error;
>> - unsigned int lookup_flags = LOOKUP_FOLLOW;
>> + unsigned int lookup_flags;
>> +
>> + if (unlikely(flags & ~AT_SYMLINK_NOFOLLOW))
>> + return -EINVAL;
>> +
>> + lookup_flags = flags & AT_SYMLINK_NOFOLLOW ? 0 : LOOKUP_FOLLOW;
>> +
>
> Why not do that in sys_fchmodat4() itself, passing lookup_flags to
> do_fchmodat() and updating old callers to pass it 0 as extra argument?
Ya, that seems better -- passing LOOKUP_FOLLOW instead of 0, to keep the
behavior the same. That way I could avoid the overhead of these checks for the
old syscalls, as we know they're not necessary.
I'll replace this patch with the following for a v3
diff --git a/fs/open.c b/fs/open.c
index b5b80469b93d..a5f99408af11 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -569,11 +569,12 @@ SYSCALL_DEFINE2(fchmod, unsigned int, fd, umode_t, mode)
return ksys_fchmod(fd, mode);
}
-int do_fchmodat(int dfd, const char __user *filename, umode_t mode)
+int do_fchmodat4(int dfd, const char __user *filename, umode_t mode,
+ int lookup_flags)
{
struct path path;
int error;
- unsigned int lookup_flags = LOOKUP_FOLLOW;
+
retry:
error = user_path_at(dfd, filename, lookup_flags, &path);
if (!error) {
@@ -587,15 +588,28 @@ int do_fchmodat(int dfd, const char __user *filename, umode_t mode)
return error;
}
+SYSCALL_DEFINE4(fchmodat4, int, dfd, const char __user *, filename,
+ umode_t, mode, int, flags)
+{
+ unsigned int lookup_flags;
+
+ if (unlikely(flags & ~AT_SYMLINK_NOFOLLOW))
+ return -EINVAL;
+
+ lookup_flags = flags & AT_SYMLINK_NOFOLLOW ? 0 : LOOKUP_FOLLOW;
+
+ return do_fchmodat4(dfd, filename, mode, lookup_flags);
+}
+
SYSCALL_DEFINE3(fchmodat, int, dfd, const char __user *, filename,
umode_t, mode)
{
- return do_fchmodat(dfd, filename, mode);
+ return do_fchmodat4(dfd, filename, mode, LOOKUP_FOLLOW);
}
SYSCALL_DEFINE2(chmod, const char __user *, filename, umode_t, mode)
{
- return do_fchmodat(AT_FDCWD, filename, mode);
+ return do_fchmodat4(AT_FDCWD, filename, mode, LOOKUP_FOLLOW);
}
static int chown_common(const struct path *path, uid_t user, gid_t group)
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index e1c20f1d0525..6676b1cc5485 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -81,6 +81,7 @@ struct io_uring_params;
#include <linux/quota.h>
#include <linux/key.h>
#include <linux/personality.h>
+#include <linux/namei.h>
#include <trace/syscall.h>
#ifdef CONFIG_ARCH_HAS_SYSCALL_WRAPPER
@@ -433,6 +434,8 @@ asmlinkage long sys_chroot(const char __user *filename);
asmlinkage long sys_fchmod(unsigned int fd, umode_t mode);
asmlinkage long sys_fchmodat(int dfd, const char __user *filename,
umode_t mode);
+asmlinkage long sys_fchmodat4(int dfd, const char __user *filename,
+ umode_t mode, int flags);
asmlinkage long sys_fchownat(int dfd, const char __user *filename, uid_t user,
gid_t group, int flag);
asmlinkage long sys_fchown(unsigned int fd, uid_t user, gid_t group);
@@ -1320,11 +1323,12 @@ static inline long ksys_link(const char __user *oldname,
return do_linkat(AT_FDCWD, oldname, AT_FDCWD, newname, 0);
}
-extern int do_fchmodat(int dfd, const char __user *filename, umode_t mode);
+extern int do_fchmodat4(int dfd, const char __user *filename, umode_t mode,
+ int flags);
static inline int ksys_chmod(const char __user *filename, umode_t mode)
{
- return do_fchmodat(AT_FDCWD, filename, mode);
+ return do_fchmodat4(AT_FDCWD, filename, mode, LOOKUP_FOLLOW);
}
extern long do_faccessat(int dfd, const char __user *filename, int mode);
^ permalink raw reply
* Re: [PATCH v2 2/4] Add fchmodat4(), a new syscall
From: Rich Felker @ 2019-07-17 2:40 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: viro, linux-kernel, linux-fsdevel, linux-api, Arnd Bergmann, rth,
ink, mattst88, linux, catalin.marinas, will, tony.luck,
fenghua.yu, geert, monstr, ralf, paul.burton, jhogan,
James.Bottomley, deller, benh, paulus, mpe, heiko.carstens, gor,
borntraeger, ysato, davem, luto, tglx, mingo, bp, hpa, x86,
peterz, acme
In-Reply-To: <20190717012719.5524-3-palmer@sifive.com>
On Tue, Jul 16, 2019 at 06:27:17PM -0700, Palmer Dabbelt wrote:
> man 3p says that fchmodat() takes a flags argument, but the Linux
> syscall does not. There doesn't appear to be a good userspace
> workaround for this issue but the implementation in the kernel is pretty
> straight-forward. The specific use case where the missing flags came up
> was WRT a fuse filesystem implemenation, but the functionality is pretty
> generic so I'm assuming there would be other use cases.
Note that we do have a workaround in musl libc with O_PATH and
/proc/self/fd, but a syscall that allows a proper fix with the ugly
workaround only in the fallback path for old kernels will be much
appreciated!
What about also doing a new SYS_faccessat4 with working AT_EACCESS
flag? The workaround we have to do for it is far worse.
Rich
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox