* Re: [PATCH] Smack: Restore the smackfsdef mount option
From: Casey Schaufler @ 2019-05-28 20:24 UTC (permalink / raw)
To: David Howells
Cc: LKML, Al Viro, jose.bollo, Linux Security Module list, casey
In-Reply-To: <26777166-5e0d-adfd-e59f-bcee7f18841a@schaufler-ca.com>
On 5/28/2019 12:57 PM, Casey Schaufler wrote:
> On 5/28/2019 11:54 AM, David Howells wrote:
>> Casey Schaufler <casey@schaufler-ca.com> wrote:
>>
>>>> Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>
>>>>>> Also, should all of these be prefixed with "smack"? So:
>>>>>>
>>>>>> fsparam_string("smackfsdef", Opt_fsdefault),
>>>>>> fsparam_string("smackfsfloor", Opt_fsfloor),
>>>>>> fsparam_string("smackfshat", Opt_fshat),
>>>>> No. smack_fs_parameters takes care of that.
>>>> It does? *Blink*.
>>> Well, something does. I can't say that I 100% understand all
>>> of how the new mount code handles the mount options. Y'all made
>>> sweeping changes, and the code works the way it used to except
>>> for the awkward change from smackfsdef to smackfsdefault. It
>>> took no small amount of head scratching and experimentation to
>>> convince myself that the fix I proposed was correct.
>> Ah... I suspect the issue is that smack_sb_eat_lsm_opts() strips the prefix
>> for an unconverted filesystem, but smack_fs_context_parse_param() doesn't
>> (which it shouldn't).
>>
>> Can you try grabbing my mount-api-viro branch from:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git
>>
>> and testing setting smack options on a tmpfs filesystem?
> My fedora system won't boot because smackfsdef isn't recognized. :(
> I will put in my fix and retry.
No joy there, either. Now it accepts smackfsdef, but doesn't
recognize smackfsroot. I don't have this problem with vanilla
5.1.
>
>> You might need to try modifying samples/vfs/test-fsmount.c to make it mount a
>> trmpfs filesystem through the new mount UAPI.
>>
>> David
^ permalink raw reply
* Re: [PATCH 4/7] vfs: Add superblock notifications
From: Jann Horn @ 2019-05-28 20:27 UTC (permalink / raw)
To: David Howells
Cc: Al Viro, raven, linux-fsdevel, Linux API, linux-block, keyrings,
linux-security-module, kernel list
In-Reply-To: <155905934373.7587.10824503964531598726.stgit@warthog.procyon.org.uk>
On Tue, May 28, 2019 at 6:05 PM David Howells <dhowells@redhat.com> wrote:
> Add a superblock event notification facility whereby notifications about
> superblock events, such as I/O errors (EIO), quota limits being hit
> (EDQUOT) and running out of space (ENOSPC) can be reported to a monitoring
> process asynchronously. Note that this does not cover vfsmount topology
> changes. mount_notify() is used for that.
[...]
> +#ifdef CONFIG_SB_NOTIFICATIONS
> +/*
> + * Post superblock notifications.
> + */
> +void post_sb_notification(struct super_block *s, struct superblock_notification *n)
> +{
> + post_watch_notification(s->s_watchers, &n->watch, current_cred(),
> + s->s_unique_id);
> +}
You're using current_cred() here? So the idea is that if some random
process runs into a disk I/O error, the I/O error will come from that
task's credentials? In general, you're not supposed to look at task
credentials in ->read/->write handlers.
> +static void release_sb_watch(struct watch_list *wlist, struct watch *watch)
> +{
> + struct super_block *s = watch->private;
> +
> + put_super(s);
> +}
> +
> +/**
> + * sys_sb_notify - Watch for superblock events.
> + * @dfd: Base directory to pathwalk from or fd referring to superblock.
> + * @filename: Path to superblock to place the watch upon
> + * @at_flags: Pathwalk control flags
> + * @watch_fd: The watch queue to send notifications to.
> + * @watch_id: The watch ID to be placed in the notification (-1 to remove watch)
> + */
> +SYSCALL_DEFINE5(sb_notify,
> + int, dfd,
> + const char __user *, filename,
> + unsigned int, at_flags,
> + int, watch_fd,
> + int, watch_id)
> +{
> + struct watch_queue *wqueue;
> + struct super_block *s;
> + struct watch_list *wlist = NULL;
> + struct watch *watch;
> + struct path path;
> + int ret;
> +
> + if (watch_id < -1 || watch_id > 0xff)
> + return -EINVAL;
> +
> + ret = user_path_at(dfd, filename, at_flags, &path);
As in the other patch, I don't think userspace is supposed to be able
to supply user_path_at()'s third argument.
It might make sense to require that the path points to the root inode
of the superblock? That way you wouldn't be able to do this on a bind
mount that exposes part of a shared filesystem to a container.
> + if (ret)
> + return ret;
> +
> + wqueue = get_watch_queue(watch_fd);
> + if (IS_ERR(wqueue))
> + goto err_path;
> +
> + s = path.dentry->d_sb;
> + if (watch_id >= 0) {
> + if (!s->s_watchers) {
> + wlist = kzalloc(sizeof(*wlist), GFP_KERNEL);
> + if (!wlist)
> + goto err_wqueue;
> + INIT_HLIST_HEAD(&wlist->watchers);
> + spin_lock_init(&wlist->lock);
> + wlist->release_watch = release_sb_watch;
> + }
> +
> + watch = kzalloc(sizeof(*watch), GFP_KERNEL);
> + if (!watch)
> + goto err_wlist;
> +
> + init_watch(watch, wqueue);
> + watch->id = s->s_unique_id;
> + watch->private = s;
> + watch->info_id = (u32)watch_id << 24;
> +
> + down_write(&s->s_umount);
> + ret = -EIO;
> + if (atomic_read(&s->s_active)) {
> + if (!s->s_watchers) {
> + s->s_watchers = wlist;
> + wlist = NULL;
> + }
> +
> + ret = add_watch_to_object(watch, s->s_watchers);
> + if (ret == 0) {
> + spin_lock(&sb_lock);
> + s->s_count++;
> + spin_unlock(&sb_lock);
Why do watches hold references on the superblock they're watching?
> + }
> + }
> + up_write(&s->s_umount);
> + if (ret < 0)
> + kfree(watch);
> + } else if (s->s_watchers) {
This should probably have something like a READ_ONCE() for clarity?
> + down_write(&s->s_umount);
> + ret = remove_watch_from_object(s->s_watchers, wqueue,
> + s->s_unique_id, false);
> + up_write(&s->s_umount);
> + } else {
> + ret = -EBADSLT;
> + }
> +
> +err_wlist:
> + kfree(wlist);
> +err_wqueue:
> + put_watch_queue(wqueue);
> +err_path:
> + path_put(&path);
> + return ret;
> +}
> +#endif
^ permalink raw reply
* Re: [PULL] Smack: Restore the smackfsdef mount option
From: Casey Schaufler @ 2019-05-28 20:37 UTC (permalink / raw)
To: David Howells
Cc: James Morris, Linux Security Module list, Al Viro, LKML, casey
In-Reply-To: <15805.1559074726@warthog.procyon.org.uk>
On 5/28/2019 1:18 PM, David Howells wrote:
> Casey Schaufler <casey@schaufler-ca.com> wrote:
>
>>> static const struct fs_parameter_spec smack_param_specs[] = {
>>> + fsparam_string("fsdef", Opt_fsdefault),
>>> fsparam_string("fsdefault", Opt_fsdefault),
>>> fsparam_string("fsfloor", Opt_fsfloor),
>>> fsparam_string("fshat", Opt_fshat),
>>>
>>> but that all the option names in that table *do* need prefixing with
>>> "smack".
> Actually, you're right, we do need to add that *and* prefix it with "smack".
>
>> I'm not sure I follow the logic, because "mount -o smackfsdefault=Pop"
>> does what I would expect it to.
> Yes, I'm sure it does - for the cases you're testing - but it's filesystem and
> syscall dependent. None of the filesystems currently ported to the mount API
> upstream override ->parse_monolithic(), but that changes with nfs, shmem and
> coda and will change with cifs too.
>
> It also changes if you use fsconfig() to supply the options because that goes
> through a different LSM hook (it uses fs_context_parse_param rather than
> sb_eat_lsm_opts).
>
>>> The way you enter the LSM is going to depend on whether
>>> generic_parse_monolithic() is called. You're only going to enter this way
>>> if mount(2) is the syscall of entry and the filesystem doesn't override
>>> the ->parse_monolithic() option (none in the upstream kernel).
>> So you're saying that the code works for the mount(2) case,
>> but won't work for some other case? Are you planning a fix?
>> Will that fix include restoration of smackfsdef?
> I can do a fix, but testing it is a pain.
I will test a fix if you point me to it. I need it for 5.1 and 5.2.
>
> David
^ permalink raw reply
* Re: [PATCH 6/7] block: Add block layer notifications
From: Jann Horn @ 2019-05-28 20:37 UTC (permalink / raw)
To: David Howells
Cc: Al Viro, raven, linux-fsdevel, Linux API, linux-block, keyrings,
linux-security-module, kernel list
In-Reply-To: <155905935953.7587.11815678364029606128.stgit@warthog.procyon.org.uk>
On Tue, May 28, 2019 at 6:05 PM David Howells <dhowells@redhat.com> wrote:
> Add a block layer notification mechanism whereby notifications about
> block-layer events such as I/O errors, can be reported to a monitoring
> process asynchronously.
[...]
> +#ifdef CONFIG_BLK_NOTIFICATIONS
> +static const enum block_notification_type blk_notifications[] = {
> + [BLK_STS_TIMEOUT] = NOTIFY_BLOCK_ERROR_TIMEOUT,
> + [BLK_STS_NOSPC] = NOTIFY_BLOCK_ERROR_NO_SPACE,
> + [BLK_STS_TRANSPORT] = NOTIFY_BLOCK_ERROR_RECOVERABLE_TRANSPORT,
> + [BLK_STS_TARGET] = NOTIFY_BLOCK_ERROR_CRITICAL_TARGET,
> + [BLK_STS_NEXUS] = NOTIFY_BLOCK_ERROR_CRITICAL_NEXUS,
> + [BLK_STS_MEDIUM] = NOTIFY_BLOCK_ERROR_CRITICAL_MEDIUM,
> + [BLK_STS_PROTECTION] = NOTIFY_BLOCK_ERROR_PROTECTION,
> + [BLK_STS_RESOURCE] = NOTIFY_BLOCK_ERROR_KERNEL_RESOURCE,
> + [BLK_STS_DEV_RESOURCE] = NOTIFY_BLOCK_ERROR_DEVICE_RESOURCE,
> + [BLK_STS_IOERR] = NOTIFY_BLOCK_ERROR_IO,
> +};
> +#endif
> +
> blk_status_t errno_to_blk_status(int errno)
> {
> int i;
> @@ -179,6 +194,19 @@ static void print_req_error(struct request *req, blk_status_t status)
> req->rq_disk ? req->rq_disk->disk_name : "?",
> (unsigned long long)blk_rq_pos(req),
> req->cmd_flags);
> +
> +#ifdef CONFIG_BLK_NOTIFICATIONS
> + if (blk_notifications[idx]) {
If you have this branch here, that indicates that blk_notifications
might be sparse - but at the same time, blk_notifications is not
defined in a way that explicitly ensures that it has as many elements
as blk_errors. It might make sense to add an explicit length to the
definition of blk_notifications - something like "static const enum
block_notification_type blk_notifications[ARRAY_SIZE(blk_errors)]"
maybe?
> + struct block_notification n = {
> + .watch.type = WATCH_TYPE_BLOCK_NOTIFY,
> + .watch.subtype = blk_notifications[idx],
> + .watch.info = sizeof(n),
> + .dev = req->rq_disk ? disk_devt(req->rq_disk) : 0,
> + .sector = blk_rq_pos(req),
> + };
> + post_block_notification(&n);
> + }
> +#endif
> }
^ permalink raw reply
* Re: [PATCH 4/7] keys: Break bits out of key_unlink()
From: James Morris @ 2019-05-28 20:41 UTC (permalink / raw)
To: David Howells; +Cc: keyrings, linux-security-module, linux-kernel
In-Reply-To: <155856411142.10428.16280282329908085391.stgit@warthog.procyon.org.uk>
On Wed, 22 May 2019, David Howells wrote:
> Break bits out of key_unlink() into helper functions so that they can be
> used in implementing key_move().
>
> Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: James Morris <jamorris@linux.microsoft.com>
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH 5/7] keys: Make __key_link_begin() handle lockdep nesting
From: James Morris @ 2019-05-28 20:42 UTC (permalink / raw)
To: David Howells; +Cc: keyrings, linux-security-module, linux-kernel
In-Reply-To: <155856411812.10428.4394700002321005951.stgit@warthog.procyon.org.uk>
On Wed, 22 May 2019, David Howells wrote:
> Make __key_link_begin() handle lockdep nesting for the implementation of
> key_move() where we have to lock two keyrings.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: James Morris <jamorris@linux.microsoft.com>
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* [PATCH] Smack: Restore the smackfsdef mount option and add missing prefixes
From: David Howells @ 2019-05-28 20:47 UTC (permalink / raw)
To: casey; +Cc: dhowells, viro, jmorris, linux-security-module, linux-kernel
From: Casey Schaufler <casey@schaufler-ca.com>
The 5.1 mount system rework changed the smackfsdef mount option
to smackfsdefault. This fixes the regression by making smackfsdef
treated the same way as smackfsdefault.
Also fix the smack_param_specs[] to have "smack" prefixes on all the names.
This isn't visible to a user unless they either:
(a) Try to mount a filesystem that's converted to the internal mount API
and that implements the ->parse_monolithic() context operation - and
only then if they call security_fs_context_parse_param() rather than
security_sb_eat_lsm_opts().
There are no examples of this upstream yet, but nfs will probably want
to do this for nfs2 or nfs3.
(b) Use fsconfig() to configure the filesystem - in which case
security_fs_context_parse_param() will be called.
This issue is that smack_sb_eat_lsm_opts() checks for the "smack" prefix on
the options, but smack_fs_context_parse_param() does not.
Fixes: c3300aaf95fb ("smack: get rid of match_token()")
Fixes: 2febd254adc4 ("smack: Implement filesystem context security hooks")
Cc: stable@vger.kernel.org
Reported-by: Jose Bollo <jose.bollo@iot.bzh>
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Signed-off-by: David Howells <dhowells@redhat.com>
---
security/smack/smack_lsm.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 0de725f88bed..d99450b4f511 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -68,6 +68,7 @@ static struct {
int len;
int opt;
} smk_mount_opts[] = {
+ {"smackfsdef", sizeof("smackfsdef") - 1, Opt_fsdefault},
A(fsdefault), A(fsfloor), A(fshat), A(fsroot), A(fstransmute)
};
#undef A
@@ -682,11 +683,12 @@ static int smack_fs_context_dup(struct fs_context *fc,
}
static const struct fs_parameter_spec smack_param_specs[] = {
- fsparam_string("fsdefault", Opt_fsdefault),
- fsparam_string("fsfloor", Opt_fsfloor),
- fsparam_string("fshat", Opt_fshat),
- fsparam_string("fsroot", Opt_fsroot),
- fsparam_string("fstransmute", Opt_fstransmute),
+ fsparam_string("smackfsdef", Opt_fsdefault),
+ fsparam_string("smackfsdefault", Opt_fsdefault),
+ fsparam_string("smackfsfloor", Opt_fsfloor),
+ fsparam_string("smackfshat", Opt_fshat),
+ fsparam_string("smackfsroot", Opt_fsroot),
+ fsparam_string("smackfstransmute", Opt_fstransmute),
{}
};
^ permalink raw reply related
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Andy Lutomirski @ 2019-05-28 20:48 UTC (permalink / raw)
To: Sean Christopherson
Cc: Xing, Cedric, Andy Lutomirski, Stephen Smalley, Jarkko Sakkinen,
James Morris, Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jethro Beekman, Hansen, Dave,
Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <20190528202407.GB13158@linux.intel.com>
On Tue, May 28, 2019 at 1:24 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Sat, May 25, 2019 at 11:09:38PM -0700, Xing, Cedric wrote:
> > > From: Andy Lutomirski [mailto:luto@kernel.org]
> > > Sent: Saturday, May 25, 2019 5:58 PM
> > >
> > > On Sat, May 25, 2019 at 3:40 PM Xing, Cedric <cedric.xing@intel.com> wrote:
> > > >
> > > > If we think of EADD as a way of mmap()'ing an enclave file into memory,
> > > > would this
> > > security_enclave_load() be the same as
> > > security_mmap_file(source_vma->vm_file, maxperm, MAP_PRIVATE), except that
> > > the target is now EPC instead of regular pages?
> > >
> > > Hmm, that's clever. Although it seems plausible that an LSM would want to
> > > allow RX or RWX of a given file page but only in the context of an approved
> > > enclave, so I think it should still be its own hook.
> >
> > What do you mean by "in the context of an approved enclave"? EPC pages are
> > *inaccessible* to any software until after EINIT. So it would never be a
> > security concern to EADD a page with wrong permissions as long as the enclave
> > would be denied eventually by LSM at EINIT.
> >
> > But I acknowledge the difference between loading a page into regular memory
> > vs. into EPC. So it's beneficial to have a separate hook, which if not
> > hooked, would pass through to security_mmap_file() by default?
>
> Mapping the enclave will still go through security_mmap_file(), the extra
> security_enclave_load() hook allows the mmap() to use PROT_NONE.
>
> > > If it's going to be in an arbitrary file, then I think the signature needs to be more like:
> > >
> > > int security_enclave_init(struct vm_area_struct *sigstruct_vma, loff_t sigstruct_offset,
> > > const sgx_sigstruct *sigstruct);
> > >
> > > So that the LSM still has the opportunity to base its decision on the contents of the
> > > SIGSTRUCT. Actually, we need that change regardless.
> >
> > Wouldn't the pair of { sigstruct_vma, sigstruct_offset } be the same as just
> > a pointer, because the VMA could be looked up using the pointer and the
> > offset would then be (pointer - vma->vm_start)?
>
> VMA has vm_file, e.g. the .sigstruct file labeled by LSMs. That being
> said, why does the LSM need the VMA? E.g. why not this?
>
> int security_enclave_init(struct file *file, struct sgx_sigstruct *sigstruct);
>
> > > > Loosely speaking, an enclave (including initial contents of all of its pages and their
> > > permissions) and its MRENCLAVE are a 1-to-1 correspondence (given the collision resistant
> > > property of SHA-2). So only one is needed for a decision, and either one would lead to the
> > > same decision. So I don't see anything making any sense here.
> > > >
> > > > Theoretically speaking, if LSM can make a decision at EINIT by means of
> > > security_enclave_load(), then security_enclave_load() is never needed.
> > > >
> > > > In practice, I support keeping both because security_enclave_load() can only approve an
> > > enumerable set while security_enclave_load() can approve a non-enumerable set of enclaves.
> > > Moreover, in order to determine the validity of a MRENCLAVE (as in development of a policy
> > > or in creation of a white/black list), system admins will need the audit log produced by
> > > security_enclave_load().
> > >
> > > I'm confused. Things like MRSIGNER aren't known until the SIGSTRUCT shows
> > > up. Also, security_enclave_load() provides no protection against loading a
> > > mishmash of two different enclave files. I see security_enclave_init() as
> > > "verify this SIGSTRUCT against your policy on who may sign enclaves and/or
> > > grant EXECMOD depending on SIGSTRUCT" and security_enclave_load() as
> > > "implement your EXECMOD / EXECUTE / WRITE / whatever policy and possibly
> > > check enclave files for some label."
> >
> > Sorry for the confusion. I was saying the same thing except that the decision
> > of security_enclave_load() doesn't have to depend on SIGSTRUCT. Given your
> > prototype of security_enclave_load(), I think we are on the same page. I made
> > the above comment to object to the idea of "require that the sigstruct be
> > supplied before any EADD operations so that the maxperm decisions can depend
> > on the sigstruct".
>
> Except that having the sigstruct allows using the sigstruct as the proxy
> for the enclave. I think the last big disconnect is that Andy and I want
> to tie everything to an enclave-specific file, i.e. sigstruct, while you
> are proposing labeling /dev/sgx/enclave. If someone wants to cram several
> sigstructs into a single file, so be it, but using /dev/sgx/enclave means
> users can't do per-enclave permissions, period.
>
> What is your objection to working on the sigstruct?
>
> > > > > > Passing both would allow tying EXECMOD to /dev/sgx/enclave as
> > > > > > Cedric wanted (without having to play games and pass
> > > > > > /dev/sgx/enclave to security_enclave_load()), but I don't think
> > > > > > there's anything fundamentally broken with using .sigstruct for
> > > > > > EXECMOD. It requires more verbose labeling, but that's not a bad thing.
> > > > >
> > > > > The benefit of putting it on .sigstruct is that it can be per-enclave.
> > > > >
> > > > > As I understand it from Fedora packaging, the way this works on
> > > > > distros is generally that a package will include some files and
> > > > > their associated labels, and, if the package needs EXECMOD, then the
> > > > > files are labeled with EXECMOD and the author of the relevant code might get a dirty
> > > look.
> > > > >
> > > > > This could translate to the author of an exclave that needs RWX
> > > > > regions getting a dirty look without leaking this permission into other enclaves.
> > > > >
> > > > > (In my opinion, the dirty looks are actually the best security
> > > > > benefit of the entire concept of LSMs making RWX difficult. A
> > > > > sufficiently creative attacker can almost always bypass W^X
> > > > > restrictions once they’ve pwned you, but W^X makes it harder to pwn
> > > > > you in the first place, and SELinux makes it really obvious when
> > > > > packaging a program that doesn’t respect W^X. The upshot is that a
> > > > > lot of programs got fixed.)
> > > >
> > > > I'm lost here. Dynamically linked enclaves, if running on SGX2, would need RW->RX, i.e.
> > > FILE__EXECMOD on /dev/sgx/enclave. But they never need RWX, i.e. PROCESS__EXECMEM.
> > >
> > > Hmm. If we want to make this distinction, we need something a big richer
> > > than my proposed callbacks. A check of the actual mprotect() / mmap()
> > > permissions would also be needed. Specifically, allowing MAXPERM=RWX
> > > wouldn't imply that PROT_WRITE | PROT_EXEC is allowed.
>
> Actually, I think we do have everything we need from an LSM perspective.
> LSMs just need to understand that sgx_enclave_load() with a NULL vma
> implies a transition from RW. For example, SELinux would interpret
> sgx_enclave_load(NULL, RX) as requiring FILE__EXECMOD.
You lost me here. What operation triggers this callback? And
wouldn't sgx_enclave_load(NULL, RX) sometimes be a transition from RO
or just some fresh executable zero bytes?
>
> As Cedric mentioned earlier, the host process doesn't necessarily know
> which pages will end up RW vs RX, i.e. sgx_enclave_load(NULL, RX)
> already has to be invoked at runtime, and when that happens, the kernel
> can take the opportunity to change the VMAs from MAY_RW to MAY_RX.
>
> For simplicity in the kernel and clarity in userspace, it makes sense to
> require an explicit ioctl() to add the to-be-EAUG'd range. That just
> leaves us wanting an ioctl() to set the post-EACCEPT{COPY} permissions.
>
> E.g.:
>
> ioctl(<prefix>_ADD_REGION, { NULL }) /* NULL == EAUG, MAY_RW */
>
> mprotect(addr, size, RW);
> ...
>
> EACCEPTCOPY -> EAUG /* page fault handler */
>
> ioctl(<prefix>_ACTIVATE_REGION, { addr, size, RX}) /* MAY_RX */
>
> mprotect(addr, size, RX);
In the maxperm model, this mprotect() will fail unless MAXPERM
contains RX, which could only happen if MAXPERM=RWX. So, regardless
of how it's actually mapped to SELinux policy, MAXPERM=RWX is
functionally like EXECMOD and actual RWX PTEs are functionally like
EXECMEM.
>
> ...
>
> And making ACTIVATE_REGION a single-shot per page eliminates the need for
> the MAXPERMS concept (see below).
>
> > If we keep only one MAXPERM, wouldn't this be the current behavior of
> > mmap()/mprotect()?
> >
> > To be a bit more clear, system admin sets MAXPERM upper bound in the form of
> > FILE__{READ|WRITE|EXECUTE|EXECMOD} of /dev/sgx/enclave. Then for a
> > process/enclave, if what it requires falls below what's allowed on
> > /dev/sgx/enclave, then everything will just work. Otherwise, it fails in the
> > form of -EPERM returned from mmap()/mprotect(). Please note that MAXPERM here
> > applies to "runtime" permissions, while "initial" permissions are taken care
> > of by security_enclave_{load|init}. "initial" permissions could be more
> > permissive than "runtime" permissions, e.g., RX is still required for initial
> > code pages even though system admins could disable dynamically loaded code
> > pages by *not* giving FILE__{EXECUTE|EXECMOD}. Therefore, the "initial"
> > mapping would still have to be done by the driver (to bypass LSM), either via
> > a new ioctl or as part of IOC_EINIT.
>
> Aha!
>
> Starting with Cedric's assertion that initial permissions can be taken
> directly from SECINFO:
>
> - Initial permissions for *EADD* pages are explicitly handled via
> sgx_enclave_load() with the exact SECINFO permissions.
>
> - Initial permissions for *EAUG* are unconditionally RW. EACCEPTCOPY
> requires the target EPC page to be RW, and EACCEPT with RO is useless.
>
> - Runtime permissions break down as follows:
> R - N/A, subset of RW (EAUG)
> W - N/A, subset of RW (EAUG) and x86 paging can't do W
> X - N/A, subset of RX (x86 paging can't do XO)
Sure it can! You just have a hypervisor that maps a PA bit to EPT
no-read. Then you can use that PA bit to suppress read. Also, Linux
already abuses PKRU to simulate XO, although that won't work for
enclaves.
> RW - Handled by EAUG LSM hook (uses RW unconditionally)
> WX - N/A, subset of RWX (x86 paging can't do WX)
> RX - Handled by ACTIVATE_REGION
> RWX - Handled by ACTIVATE_REGION
>
> In other words, if we define the SGX -> LSM calls as follows (minus the
> file pointer and other params for brevity):
>
> - <prefix>_ACTIVATE_REGION(vma, perms) -> sgx_enclave_load(NULL, perms)
>
> - <prefix>_ADD_REGION(vma) -> sgx_enclave_load(vma, SECINFO.perms)
>
> - <prefix>_ADD_REGION(NULL) -> sgx_enclave_load(NULL, RW)
>
> then SGX and LSMs have all the information and hooks needed. The catch
> is that the LSM semantics of sgx_enclave_load(..., RW) would need to be
> different than normal shared memory, e.g. FILE__WRITE should *not* be
> required, but that's ok since it's an SGX specific hook. And if for some
> reason an LSM wanted to gate access to EAUG *without* FILE__EXECMOD, it'd
> have the necessary information to do so.
>
> The userspace changes are fairly minimal:
>
> - For SGX1, use PROT_NONE for the initial mmap() and refactor ADD_PAGE
> to ADD_REGION.
>
> - For SGX2, do an explicit ADD_REGION on the ranges to be EAUG'd, and an
> ACTIVATE_REGION to make a region RX or R (no extra ioctl() required to
> keep RW permissions).
>
> Because ACTIVATE_REGION can only be done once per page, to do *abitrary*
> mprotect() transitions, userspace would need to set the added/activated
> permissions to be a superset of the transitions, e.g. RW -> RX would
> require RWX, but that's a non-issue.
>
I may be misunderstanding or just be biased to my own proposal, but
this seems potentially more complicated and less flexible than the
MAXPERM model. One of the main things that made me come up with
MAXPERM is that I wanted to avoid any complicated PTE/VMA modification
or runtime changes. So, with MAXPERM, we still need to track the
MAXPERM bits per page, but we don't ever need to *change* them or to
worry about what is or is not mapped anywhere at any given time. With
ACTIVATE_REGION, don't we need to make sure that we don't have a
second VMA pointing at the same pages? Or am I just confused?
> - For SGX1 it's a nop since it's impossible to change the EPCM
> permissions, i.e. the page would need to be RWX regardless.
I may still be missing something, but, for SGX1, it's possible at
least in principle for the enclave to request, via ocall or similar,
that the untrusted runtime do mprotect(). It's not even such a bad
idea. Honestly, enclaves *shouldn't* have anything actually writable
and executable at once because the enclaves don't want to be easily
exploited.
>
> - For SGX2, userspace can suck it up and request RWX to do completely
> arbitrary transitions (working as intended), or the kernel can support
> trimming (removing) pages from an enclave, which would allow userspace
> to do "arbitrary" transitions by first removing the page.
^ permalink raw reply
* Re: [PATCH 6/7] keys: Add a keyctl to move a key between keyrings
From: James Morris @ 2019-05-28 20:51 UTC (permalink / raw)
To: David Howells; +Cc: keyrings, linux-security-module, linux-kernel
In-Reply-To: <155856412507.10428.15987388402707639951.stgit@warthog.procyon.org.uk>
On Wed, 22 May 2019, David Howells wrote:
> +
> + if (flags & ~KEYCTL_MOVE_EXCL)
> + return -EINVAL;
> +
> + key_ref = lookup_user_key(id, KEY_LOOKUP_CREATE, KEY_NEED_LINK);
> + if (IS_ERR(key_ref)) {
> + ret = PTR_ERR(key_ref);
> + goto error;
> + }
This could probably be a simple return, as there is no cleanup.
> +
> + from_ref = lookup_user_key(from_ringid, 0, KEY_NEED_WRITE);
> + if (IS_ERR(from_ref)) {
> + ret = PTR_ERR(from_ref);
> + goto error2;
> + }
> +
> + to_ref = lookup_user_key(to_ringid, KEY_LOOKUP_CREATE, KEY_NEED_WRITE);
> + if (IS_ERR(to_ref)) {
> + ret = PTR_ERR(to_ref);
> + goto error3;
> + }
> +
> + ret = key_move(key_ref_to_ptr(key_ref), key_ref_to_ptr(from_ref),
> + key_ref_to_ptr(to_ref), flags);
> +
> + key_ref_put(to_ref);
> +error3:
> + key_ref_put(from_ref);
> +error2:
> + key_ref_put(key_ref);
> +error:
> + return ret;
> +}
> +
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH 7/7] keys: Grant Link permission to possessers of request_key auth keys
From: James Morris @ 2019-05-28 21:01 UTC (permalink / raw)
To: David Howells; +Cc: keyrings, linux-security-module, linux-kernel
In-Reply-To: <155856413201.10428.13385006340817641517.stgit@warthog.procyon.org.uk>
On Wed, 22 May 2019, David Howells wrote:
> Grant Link permission to the possessers of request_key authentication keys,
> thereby allowing a daemon that is servicing upcalls to arrange things such
> that only the necessary auth key is passed to the actual service program
> and not all the daemon's pending auth keys.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: James Morris <jamorris@linux.microsoft.com>
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH] Smack: Restore the smackfsdef mount option and add missing prefixes
From: Casey Schaufler @ 2019-05-28 21:39 UTC (permalink / raw)
To: David Howells; +Cc: viro, jmorris, linux-security-module, linux-kernel, casey
In-Reply-To: <155907646050.25083.16573974978890009010.stgit@warthog.procyon.org.uk>
On 5/28/2019 1:47 PM, David Howells wrote:
> From: Casey Schaufler <casey@schaufler-ca.com>
>
> The 5.1 mount system rework changed the smackfsdef mount option
> to smackfsdefault. This fixes the regression by making smackfsdef
> treated the same way as smackfsdefault.
>
> Also fix the smack_param_specs[] to have "smack" prefixes on all the names.
> This isn't visible to a user unless they either:
>
> (a) Try to mount a filesystem that's converted to the internal mount API
> and that implements the ->parse_monolithic() context operation - and
> only then if they call security_fs_context_parse_param() rather than
> security_sb_eat_lsm_opts().
>
> There are no examples of this upstream yet, but nfs will probably want
> to do this for nfs2 or nfs3.
>
> (b) Use fsconfig() to configure the filesystem - in which case
> security_fs_context_parse_param() will be called.
>
> This issue is that smack_sb_eat_lsm_opts() checks for the "smack" prefix on
> the options, but smack_fs_context_parse_param() does not.
>
> Fixes: c3300aaf95fb ("smack: get rid of match_token()")
> Fixes: 2febd254adc4 ("smack: Implement filesystem context security hooks")
> Cc: stable@vger.kernel.org
> Reported-by: Jose Bollo <jose.bollo@iot.bzh>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
Tested-by: Casey Schaufler <casey@schaufler-ca.com>
Looks good. Can you send this in for 5.1 and 5.2?
> ---
>
> security/smack/smack_lsm.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 0de725f88bed..d99450b4f511 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -68,6 +68,7 @@ static struct {
> int len;
> int opt;
> } smk_mount_opts[] = {
> + {"smackfsdef", sizeof("smackfsdef") - 1, Opt_fsdefault},
> A(fsdefault), A(fsfloor), A(fshat), A(fsroot), A(fstransmute)
> };
> #undef A
> @@ -682,11 +683,12 @@ static int smack_fs_context_dup(struct fs_context *fc,
> }
>
> static const struct fs_parameter_spec smack_param_specs[] = {
> - fsparam_string("fsdefault", Opt_fsdefault),
> - fsparam_string("fsfloor", Opt_fsfloor),
> - fsparam_string("fshat", Opt_fshat),
> - fsparam_string("fsroot", Opt_fsroot),
> - fsparam_string("fstransmute", Opt_fstransmute),
> + fsparam_string("smackfsdef", Opt_fsdefault),
> + fsparam_string("smackfsdefault", Opt_fsdefault),
> + fsparam_string("smackfsfloor", Opt_fsfloor),
> + fsparam_string("smackfshat", Opt_fshat),
> + fsparam_string("smackfsroot", Opt_fsroot),
> + fsparam_string("smackfstransmute", Opt_fstransmute),
> {}
> };
>
>
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Sean Christopherson @ 2019-05-28 21:41 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Xing, Cedric, Stephen Smalley, Jarkko Sakkinen, James Morris,
Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jethro Beekman, Hansen, Dave,
Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <CALCETrWTXCb1jru1G5G3sOp5AV8iYUtrffiSxE-5gotXtrZD-g@mail.gmail.com>
On Tue, May 28, 2019 at 01:48:02PM -0700, Andy Lutomirski wrote:
> On Tue, May 28, 2019 at 1:24 PM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> >
> > Actually, I think we do have everything we need from an LSM perspective.
> > LSMs just need to understand that sgx_enclave_load() with a NULL vma
> > implies a transition from RW. For example, SELinux would interpret
> > sgx_enclave_load(NULL, RX) as requiring FILE__EXECMOD.
>
> You lost me here. What operation triggers this callback? And
> wouldn't sgx_enclave_load(NULL, RX) sometimes be a transition from RO
> or just some fresh executable zero bytes?
An explicit ioctl() after EACCEPTCOPY to update the allowed permissions.
For all intents and purposes, the EAUG'd page must start RW. Maybe a
better way to phrase it is that at some point the page must be writable
to have any value whatsover. EACCEPTCOPY explicitly requires the page to
be at least RW. EACCEPT technically doesn't require RW, but a RO or RX
zero page is useless. Userspace could still EACCEPT with RO or RX, but
SGX would assume a minimum of RW for the purposes of the LSM check.
> > As Cedric mentioned earlier, the host process doesn't necessarily know
> > which pages will end up RW vs RX, i.e. sgx_enclave_load(NULL, RX)
> > already has to be invoked at runtime, and when that happens, the kernel
> > can take the opportunity to change the VMAs from MAY_RW to MAY_RX.
> >
> > For simplicity in the kernel and clarity in userspace, it makes sense to
> > require an explicit ioctl() to add the to-be-EAUG'd range. That just
> > leaves us wanting an ioctl() to set the post-EACCEPT{COPY} permissions.
> >
> > E.g.:
> >
> > ioctl(<prefix>_ADD_REGION, { NULL }) /* NULL == EAUG, MAY_RW */
> >
> > mprotect(addr, size, RW);
> > ...
> >
> > EACCEPTCOPY -> EAUG /* page fault handler */
> >
> > ioctl(<prefix>_ACTIVATE_REGION, { addr, size, RX}) /* MAY_RX */
> >
> > mprotect(addr, size, RX);
>
> In the maxperm model, this mprotect() will fail unless MAXPERM
> contains RX, which could only happen if MAXPERM=RWX. So, regardless
> of how it's actually mapped to SELinux policy, MAXPERM=RWX is
> functionally like EXECMOD and actual RWX PTEs are functionally like
> EXECMEM.
Yep, same idea, except in the proposed flow ACTIVATE_REGION.
> > ...
> >
> > And making ACTIVATE_REGION a single-shot per page eliminates the need for
> > the MAXPERMS concept (see below).
> >
> > > If we keep only one MAXPERM, wouldn't this be the current behavior of
> > > mmap()/mprotect()?
> > >
> > > To be a bit more clear, system admin sets MAXPERM upper bound in the form of
> > > FILE__{READ|WRITE|EXECUTE|EXECMOD} of /dev/sgx/enclave. Then for a
> > > process/enclave, if what it requires falls below what's allowed on
> > > /dev/sgx/enclave, then everything will just work. Otherwise, it fails in the
> > > form of -EPERM returned from mmap()/mprotect(). Please note that MAXPERM here
> > > applies to "runtime" permissions, while "initial" permissions are taken care
> > > of by security_enclave_{load|init}. "initial" permissions could be more
> > > permissive than "runtime" permissions, e.g., RX is still required for initial
> > > code pages even though system admins could disable dynamically loaded code
> > > pages by *not* giving FILE__{EXECUTE|EXECMOD}. Therefore, the "initial"
> > > mapping would still have to be done by the driver (to bypass LSM), either via
> > > a new ioctl or as part of IOC_EINIT.
> >
> > Aha!
> >
> > Starting with Cedric's assertion that initial permissions can be taken
> > directly from SECINFO:
> >
> > - Initial permissions for *EADD* pages are explicitly handled via
> > sgx_enclave_load() with the exact SECINFO permissions.
> >
> > - Initial permissions for *EAUG* are unconditionally RW. EACCEPTCOPY
> > requires the target EPC page to be RW, and EACCEPT with RO is useless.
> >
> > - Runtime permissions break down as follows:
> > R - N/A, subset of RW (EAUG)
> > W - N/A, subset of RW (EAUG) and x86 paging can't do W
> > X - N/A, subset of RX (x86 paging can't do XO)
>
> Sure it can! You just have a hypervisor that maps a PA bit to EPT
> no-read. Then you can use that PA bit to suppress read. Also, Linux
> already abuses PKRU to simulate XO, although that won't work for
> enclaves.
Heh, I intentionally said "x86 paging" to rule out EPT :-) I'm pretty
sure it's a moot point though, I have a hard time believing an LSM will
allow RW->X and not RW->RX.
> > RW - Handled by EAUG LSM hook (uses RW unconditionally)
> > WX - N/A, subset of RWX (x86 paging can't do WX)
> > RX - Handled by ACTIVATE_REGION
> > RWX - Handled by ACTIVATE_REGION
> >
> > In other words, if we define the SGX -> LSM calls as follows (minus the
> > file pointer and other params for brevity):
> >
> > - <prefix>_ACTIVATE_REGION(vma, perms) -> sgx_enclave_load(NULL, perms)
> >
> > - <prefix>_ADD_REGION(vma) -> sgx_enclave_load(vma, SECINFO.perms)
> >
> > - <prefix>_ADD_REGION(NULL) -> sgx_enclave_load(NULL, RW)
> >
> > then SGX and LSMs have all the information and hooks needed. The catch
> > is that the LSM semantics of sgx_enclave_load(..., RW) would need to be
> > different than normal shared memory, e.g. FILE__WRITE should *not* be
> > required, but that's ok since it's an SGX specific hook. And if for some
> > reason an LSM wanted to gate access to EAUG *without* FILE__EXECMOD, it'd
> > have the necessary information to do so.
> >
> > The userspace changes are fairly minimal:
> >
> > - For SGX1, use PROT_NONE for the initial mmap() and refactor ADD_PAGE
> > to ADD_REGION.
> >
> > - For SGX2, do an explicit ADD_REGION on the ranges to be EAUG'd, and an
> > ACTIVATE_REGION to make a region RX or R (no extra ioctl() required to
> > keep RW permissions).
> >
> > Because ACTIVATE_REGION can only be done once per page, to do *abitrary*
> > mprotect() transitions, userspace would need to set the added/activated
> > permissions to be a superset of the transitions, e.g. RW -> RX would
> > require RWX, but that's a non-issue.
> >
>
> I may be misunderstanding or just be biased to my own proposal, but
> this seems potentially more complicated and less flexible than the
> MAXPERM model. One of the main things that made me come up with
> MAXPERM is that I wanted to avoid any complicated PTE/VMA modification
> or runtime changes. So, with MAXPERM, we still need to track the
> MAXPERM bits per page, but we don't ever need to *change* them or to
> worry about what is or is not mapped anywhere at any given time. With
> ACTIVATE_REGION, don't we need to make sure that we don't have a
> second VMA pointing at the same pages? Or am I just confused?
In theory, it's still your MAXPERM model, but with the unnecessary states
removed and the others enforced/handled by the natural SGX transitions
instead of explictly in ioctls. Underneath the hood the SGX driver would
still need to track the MAXPERM.
With SGX1, SECINFO == MAXPERM. With SGX2, ACTIVATE_REGION == MAXPERM,
with the implication that the previous state is always RW.
> > - For SGX1 it's a nop since it's impossible to change the EPCM
> > permissions, i.e. the page would need to be RWX regardless.
>
> I may still be missing something, but, for SGX1, it's possible at
> least in principle for the enclave to request, via ocall or similar,
> that the untrusted runtime do mprotect(). It's not even such a bad
> idea. Honestly, enclaves *shouldn't* have anything actually writable
> and executable at once because the enclaves don't want to be easily
> exploited.
Yes, but the *EPCM* permissions are immutable. So if an enclave wants
to do RW->RX it has to intialize its pages to RWX. And because the
untrusted runtime is, ahem, untrusted, the enclave cannot rely on
userspace to never map its pages RWX. In other words, from a enclave
security perspective, an SGX1 enclave+runtime that uses RW->RX is no
different than an enclave that uses RWX. Using your earlier terminology,
an SGX1 enclave *should* get a dirty looks if maps a page RWX in the EPCM,
even if it only intends RW->RX behavior.
> > - For SGX2, userspace can suck it up and request RWX to do completely
> > arbitrary transitions (working as intended), or the kernel can support
> > trimming (removing) pages from an enclave, which would allow userspace
> > to do "arbitrary" transitions by first removing the page.
^ permalink raw reply
* Re: [PATCH 1/7] General notification queue with user mmap()'able ring buffer
From: David Howells @ 2019-05-28 22:28 UTC (permalink / raw)
To: Jann Horn
Cc: dhowells, Al Viro, raven, linux-fsdevel, Linux API, linux-block,
keyrings, linux-security-module, kernel list
In-Reply-To: <CAG48ez3L5KzKyKMxUTaaB=r1E1ZXh=m6e9+CwYcXfRnUSjDvWA@mail.gmail.com>
Jann Horn <jannh@google.com> wrote:
> I don't see you setting any special properties on the VMA that would
> prevent userspace from extending its size via mremap() - no
> VM_DONTEXPAND or VM_PFNMAP. So I think you might get an out-of-bounds
> access here?
Should I just set VM_DONTEXPAND in watch_queue_mmap()? Like so:
vma->vm_flags |= VM_DONTEXPAND;
> > +static void watch_queue_map_pages(struct vm_fault *vmf,
> > + pgoff_t start_pgoff, pgoff_t end_pgoff)
> ...
>
> Same as above.
Same solution as above? Or do I need ot check start/end_pgoff too?
> > +static int watch_queue_mmap(struct file *file, struct vm_area_struct *vma)
> > +{
> > + struct watch_queue *wqueue = file->private_data;
> > +
> > + if (vma->vm_pgoff != 0 ||
> > + vma->vm_end - vma->vm_start > wqueue->nr_pages * PAGE_SIZE ||
> > + !(pgprot_val(vma->vm_page_prot) & pgprot_val(PAGE_SHARED)))
> > + return -EINVAL;
>
> This thing should probably have locking against concurrent
> watch_queue_set_size()?
Yeah.
static int watch_queue_mmap(struct file *file,
struct vm_area_struct *vma)
{
struct watch_queue *wqueue = file->private_data;
struct inode *inode = file_inode(file);
u8 nr_pages;
inode_lock(inode);
nr_pages = wqueue->nr_pages;
inode_unlock(inode);
if (nr_pages == 0 ||
...
return -EINVAL;
> > + smp_store_release(&buf->meta.head, len);
>
> Why is this an smp_store_release()? The entire buffer should not be visible to
> userspace before this setup is complete, right?
Yes - if I put the above locking in the mmap handler. OTOH, it's a one-off
barrier...
> > + if (wqueue->buffer)
> > + return -EBUSY;
>
> The preceding check occurs without any locks held and therefore has no
> reliable effect. It should probably be moved below the
> inode_lock(...).
Yeah, it can race. I'll move it into watch_queue_set_size().
> > +static void free_watch(struct rcu_head *rcu)
> > +{
> > + struct watch *watch = container_of(rcu, struct watch, rcu);
> > +
> > + put_watch_queue(rcu_access_pointer(watch->queue));
>
> This should be rcu_dereference_protected(..., 1).
That shouldn't be necessary. rcu_access_pointer()'s description says:
* It is also permissible to use rcu_access_pointer() when read-side
* access to the pointer was removed at least one grace period ago, as
* is the case in the context of the RCU callback that is freeing up
* the data, ...
It's in an rcu callback function, so accessing the __rcu pointers in the RCU'd
struct should be fine with rcu_access_pointer().
> > + /* We don't need the watch list lock for the next bit as RCU is
> > + * protecting everything from being deallocated.
>
> Does "everything" mean "the wqueue" or more than that?
Actually, just 'wqueue' and its buffer. 'watch' is held by us once we've
dequeued it as we now own the ref 'wlist' had on it. 'wlist' and 'wq' must be
pinned by the caller.
> > + if (release) {
> > + if (wlist->release_watch) {
> > + rcu_read_unlock();
> > + /* This might need to call dput(), so
> > + * we have to drop all the locks.
> > + */
> > + wlist->release_watch(wlist, watch);
>
> How are you holding a reference to `wlist` here? You got the reference through
> rcu_dereference(), you've dropped the RCU read lock, and I don't see anything
> that stabilizes the reference.
The watch record must hold a ref on the watched object if the watch_list has a
->release_watch() method. In the code snippet above, the watch record now
belongs to us because we unlinked it under the wlist->lock some lines prior.
However, you raise a good point, and I think the thing to do is to cache
->release_watch from it and not pass wlist into (*release_watch)(). We don't
need to concern ourselves with cleaning up *wlist as it will be cleaned up
when the target object is removed.
Keyrings don't have a ->release_watch method and neither does the block-layer
notification stuff.
> > + if (wqueue->pages && wqueue->pages[0])
> > + WARN_ON(page_ref_count(wqueue->pages[0]) != 1);
>
> Is there a reason why there couldn't still be references to the pages
> from get_user_pages()/get_user_pages_fast()?
I'm not sure. I'm not sure what to do if there are. What do you suggest?
> > + n->info &= (WATCH_INFO_LENGTH | WATCH_INFO_TYPE_FLAGS | WATCH_INFO_ID);
>
> Should the non-atomic modification of n->info
n's an unpublished copy of some userspace data that's local to the function
instance. There shouldn't be any way to race with it at this point.
> (and perhaps also the
> following uses of ->debug) be protected by some lock?
>
> > + if (post_one_notification(wqueue, n, file->f_cred))
> > + wqueue->debug = 0;
> > + else
> > + wqueue->debug++;
> > + ret = len;
> > + if (wqueue->debug > 20)
> > + ret = -EIO;
It's for debugging purposes, so the non-atomiticity doesn't matter. I'll
#undef the symbol to disable the code.
> > +#define IOC_WATCH_QUEUE_SET_SIZE _IO('s', 0x01) /* Set the size in pages */
> > +#define IOC_WATCH_QUEUE_SET_FILTER _IO('s', 0x02) /* Set the filter */
>
> Should these ioctl numbers be registered in
> Documentation/ioctl/ioctl-number.txt?
Quite possibly. I'm not sure where's best to actually allocate it. I picked
's' out of the air.
David
^ permalink raw reply
* Re: [PATCH 3/7] vfs: Add a mount-notification facility
From: David Howells @ 2019-05-28 23:04 UTC (permalink / raw)
To: Jann Horn
Cc: dhowells, Al Viro, raven, linux-fsdevel, Linux API, linux-block,
keyrings, linux-security-module, kernel list
In-Reply-To: <CAG48ez2rRh2_Kq_EGJs5k-ZBNffGs_Q=vkQdinorBgo58tbGpg@mail.gmail.com>
Jann Horn <jannh@google.com> wrote:
> It might make sense to redesign this stuff so that watches don't hold
> references on the object being watched.
I explicitly made it hold a reference so that if you place a watch on an
automounted mount it stops it from expiring.
Further, if I create a watch on something, *should* it be unmountable, just as
if I had a file open there or had chdir'd into there?
David
^ permalink raw reply
* Re: [PATCH 3/7] vfs: Add a mount-notification facility
From: David Howells @ 2019-05-28 23:08 UTC (permalink / raw)
To: Jann Horn
Cc: dhowells, Al Viro, raven, linux-fsdevel, Linux API, linux-block,
keyrings, linux-security-module, kernel list
In-Reply-To: <10418.1559084686@warthog.procyon.org.uk>
David Howells <dhowells@redhat.com> wrote:
> > It might make sense to redesign this stuff so that watches don't hold
> > references on the object being watched.
>
> I explicitly made it hold a reference so that if you place a watch on an
> automounted mount it stops it from expiring.
>
> Further, if I create a watch on something, *should* it be unmountable, just as
> if I had a file open there or had chdir'd into there?
It gets trickier than that as I need a ref on the dentry on which the watch is
rooted to prevent it from getting culled.
David
^ permalink raw reply
* Re: [PATCH 1/7] General notification queue with user mmap()'able ring buffer
From: Greg KH @ 2019-05-28 23:12 UTC (permalink / raw)
To: David Howells
Cc: viro, raven, linux-fsdevel, linux-api, linux-block, keyrings,
linux-security-module, linux-kernel
In-Reply-To: <4031.1559064620@warthog.procyon.org.uk>
On Tue, May 28, 2019 at 06:30:20PM +0100, David Howells wrote:
> Greg KH <gregkh@linuxfoundation.org> wrote:
>
> > > Implement a misc device that implements a general notification queue as a
> > > ring buffer that can be mmap()'d from userspace.
> >
> > "general" but just for filesystems, right? :(
>
> Whatever gave you that idea? You can watch keyrings events, for example -
> they're not exactly filesystems. I've added the ability to watch for mount
> topology changes and superblock events because those are something I've been
> asked to do. I've added something for block events because I've recently had
> a problem with trying to recover data from a dodgy disk in that every time the
> disk goes offline, the ddrecover goes "wheeeee!" as it just sees a lot of
> EIO/ENODATA at a great rate of knots because it doesn't know the driver is now
> ignoring the disk.
>
> I don't know what else people might want to watch, but I've tried to make it
> as generic as possible so as not to exclude it if possible.
Ok, let me try to dig up some older proposals to see if this fits into
the same model to work with them as well.
> > > + refcount_t usage;
> >
> > Usage of what, this structure? Or something else?
>
> This is the number of usages of this struct (references to if you prefer). I
> can add a comment to this effect.
I think you answer this later on with the kref comment :)
> > > + return -EOPNOTSUPP;
> >
> > -ENOTTY is the correct "not a valid ioctl" error value, right?
>
> fs/ioctl.c does both, but I can switch it if it makes you happier.
It does.
> > > +void put_watch_queue(struct watch_queue *wqueue)
> > > +{
> > > + if (refcount_dec_and_test(&wqueue->usage))
> > > + kfree_rcu(wqueue, rcu);
> >
> > Why not just use a kref?
>
> Why use a kref? It seems like an effort to be a C++ base class, but without
> the C++ inheritance bit. Using kref doesn't seem to gain anything. It's just
> a wrapper around refcount_t - so why not just use a refcount_t?
>
> kref_put() could potentially add an unnecessary extra stack frame and would
> seem to be best avoided, though an optimising compiler ought to be able to
> inline if it can.
If kref_put() is on your fast path, you have worse problems (kfree isn't
fast, right?)
Anyway, it's an inline function, how can it add an extra stack frame?
Don't try to optimize something that isn't needed yet.
> Are you now on the convert all refcounts to krefs path?
"now"? Remember, I wrote kref all those years ago, everyone should use
it. It saves us having to audit the same pattern over and over again.
And, even nicer, it uses a refcount now, and as you are trying to
reference count an object, it is exactly what this was written for.
So yes, I do think it should be used here, unless it is deemed to not
fit the pattern/usage model.
> > > +EXPORT_SYMBOL(add_watch_to_object);
> >
> > Naming nit, shouldn't the "prefix" all be the same for these new
> > functions?
> >
> > watch_queue_add_object()? watch_queue_put()? And so on?
>
> Naming is fun. watch_queue_add_object - that suggests something different to
> what the function actually does. I'll think about adjusting the names.
Ok, just had to say something. It's your call, and yes, naming is hard.
> > > +module_exit(watch_queue_exit);
> >
> > module_misc_device()?
>
> warthog>git grep module_misc_device -- Documentation/
> warthog1>
Do I have to document all helper macros? Anyway, it saves you
boilerplate code, but if built in, it's at the module init level, not
the fs init level, like you are asking for here. So that might not
work, it's your call.
> > > + struct {
> > > + struct watch_notification watch; /* WATCH_TYPE_SKIP */
> > > + volatile __u32 head; /* Ring head index */
> > > + volatile __u32 tail; /* Ring tail index */
> >
> > A uapi structure that has volatile in it? Are you _SURE_ this is
> > correct?
> >
> > That feels wrong to me... This is not a backing-hardware register, it's
> > "just memory" and slapping volatile on it shouldn't be the correct
> > solution for telling the compiler to not to optimize away reads/flushes,
> > right? You need a proper memory access type primitive for that to work
> > correctly everywhere I thought.
> >
> > We only have 2 users of volatile in include/uapi, one for WMI structures
> > that are backed by firmware (seems correct), and one for DRM which I
> > have no idea how it works as it claims to be a lock. Why is this new
> > addition the correct way to do this that no other ring-buffer that was
> > mmapped has needed to?
>
> Yeah, I understand your concern with this.
>
> The reason I put the volatiles in is that the kernel may be modifying the head
> pointer on one CPU simultaneously with userspace modifying the tail pointer on
> another CPU.
>
> Note that userspace does not need to enter the kernel to find out if there's
> anything in the buffer or to read stuff out of the buffer. Userspace only
> needs to enter the kernel, using poll() or similar, to wait for something to
> appear in the buffer.
And how does the tracing and perf ring buffers do this without needing
volatile? Why not use the same type of interface they provide, as it's
always good to share code that has already had all of the nasty corner
cases worked out.
Anyway, I don't want you to think I don't like this code/idea overall, I
do. I just want to see it be something that everyone can use, and use
easily, as it has been something lots of people have been asking for for
a long time.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 1/7] General notification queue with user mmap()'able ring buffer
From: Jann Horn @ 2019-05-28 23:16 UTC (permalink / raw)
To: David Howells
Cc: Al Viro, raven, linux-fsdevel, Linux API, linux-block, keyrings,
linux-security-module, kernel list
In-Reply-To: <11466.1559082515@warthog.procyon.org.uk>
On Wed, May 29, 2019 at 12:28 AM David Howells <dhowells@redhat.com> wrote:
> Jann Horn <jannh@google.com> wrote:
> > I don't see you setting any special properties on the VMA that would
> > prevent userspace from extending its size via mremap() - no
> > VM_DONTEXPAND or VM_PFNMAP. So I think you might get an out-of-bounds
> > access here?
>
> Should I just set VM_DONTEXPAND in watch_queue_mmap()? Like so:
>
> vma->vm_flags |= VM_DONTEXPAND;
Yeah, that should work.
> > > +static void watch_queue_map_pages(struct vm_fault *vmf,
> > > + pgoff_t start_pgoff, pgoff_t end_pgoff)
> > ...
> >
> > Same as above.
>
> Same solution as above? Or do I need ot check start/end_pgoff too?
Same solution.
> > > +static int watch_queue_mmap(struct file *file, struct vm_area_struct *vma)
> > > +{
> > > + struct watch_queue *wqueue = file->private_data;
> > > +
> > > + if (vma->vm_pgoff != 0 ||
> > > + vma->vm_end - vma->vm_start > wqueue->nr_pages * PAGE_SIZE ||
> > > + !(pgprot_val(vma->vm_page_prot) & pgprot_val(PAGE_SHARED)))
> > > + return -EINVAL;
> >
> > This thing should probably have locking against concurrent
> > watch_queue_set_size()?
>
> Yeah.
>
> static int watch_queue_mmap(struct file *file,
> struct vm_area_struct *vma)
> {
> struct watch_queue *wqueue = file->private_data;
> struct inode *inode = file_inode(file);
> u8 nr_pages;
>
> inode_lock(inode);
> nr_pages = wqueue->nr_pages;
> inode_unlock(inode);
>
> if (nr_pages == 0 ||
> ...
> return -EINVAL;
Looks reasonable.
> > > + smp_store_release(&buf->meta.head, len);
> >
> > Why is this an smp_store_release()? The entire buffer should not be visible to
> > userspace before this setup is complete, right?
>
> Yes - if I put the above locking in the mmap handler. OTOH, it's a one-off
> barrier...
>
> > > + if (wqueue->buffer)
> > > + return -EBUSY;
> >
> > The preceding check occurs without any locks held and therefore has no
> > reliable effect. It should probably be moved below the
> > inode_lock(...).
>
> Yeah, it can race. I'll move it into watch_queue_set_size().
Sounds good.
> > > +static void free_watch(struct rcu_head *rcu)
> > > +{
> > > + struct watch *watch = container_of(rcu, struct watch, rcu);
> > > +
> > > + put_watch_queue(rcu_access_pointer(watch->queue));
> >
> > This should be rcu_dereference_protected(..., 1).
>
> That shouldn't be necessary. rcu_access_pointer()'s description says:
>
> * It is also permissible to use rcu_access_pointer() when read-side
> * access to the pointer was removed at least one grace period ago, as
> * is the case in the context of the RCU callback that is freeing up
> * the data, ...
>
> It's in an rcu callback function, so accessing the __rcu pointers in the RCU'd
> struct should be fine with rcu_access_pointer().
Aah, whoops, you're right, I missed that paragraph in the
documentation of rcu_access_pointer().
> > > + /* We don't need the watch list lock for the next bit as RCU is
> > > + * protecting everything from being deallocated.
> >
> > Does "everything" mean "the wqueue" or more than that?
>
> Actually, just 'wqueue' and its buffer. 'watch' is held by us once we've
> dequeued it as we now own the ref 'wlist' had on it. 'wlist' and 'wq' must be
> pinned by the caller.
>
> > > + if (release) {
> > > + if (wlist->release_watch) {
> > > + rcu_read_unlock();
> > > + /* This might need to call dput(), so
> > > + * we have to drop all the locks.
> > > + */
> > > + wlist->release_watch(wlist, watch);
> >
> > How are you holding a reference to `wlist` here? You got the reference through
> > rcu_dereference(), you've dropped the RCU read lock, and I don't see anything
> > that stabilizes the reference.
>
> The watch record must hold a ref on the watched object if the watch_list has a
> ->release_watch() method. In the code snippet above, the watch record now
> belongs to us because we unlinked it under the wlist->lock some lines prior.
Ah, of course.
> However, you raise a good point, and I think the thing to do is to cache
> ->release_watch from it and not pass wlist into (*release_watch)(). We don't
> need to concern ourselves with cleaning up *wlist as it will be cleaned up
> when the target object is removed.
>
> Keyrings don't have a ->release_watch method and neither does the block-layer
> notification stuff.
>
> > > + if (wqueue->pages && wqueue->pages[0])
> > > + WARN_ON(page_ref_count(wqueue->pages[0]) != 1);
> >
> > Is there a reason why there couldn't still be references to the pages
> > from get_user_pages()/get_user_pages_fast()?
>
> I'm not sure. I'm not sure what to do if there are. What do you suggest?
I would use put_page() instead of manually freeing it; I think that
should be enough? I'm not entirely sure though.
> > > + n->info &= (WATCH_INFO_LENGTH | WATCH_INFO_TYPE_FLAGS | WATCH_INFO_ID);
> >
> > Should the non-atomic modification of n->info
>
> n's an unpublished copy of some userspace data that's local to the function
> instance. There shouldn't be any way to race with it at this point.
Ah, derp. Yes.
^ permalink raw reply
* Re: [PATCH 3/7] vfs: Add a mount-notification facility
From: Jann Horn @ 2019-05-28 23:23 UTC (permalink / raw)
To: David Howells
Cc: Al Viro, raven, linux-fsdevel, Linux API, linux-block, keyrings,
linux-security-module, kernel list
In-Reply-To: <10418.1559084686@warthog.procyon.org.uk>
On Wed, May 29, 2019 at 1:04 AM David Howells <dhowells@redhat.com> wrote:
> Jann Horn <jannh@google.com> wrote:
> > It might make sense to redesign this stuff so that watches don't hold
> > references on the object being watched.
>
> I explicitly made it hold a reference so that if you place a watch on an
> automounted mount it stops it from expiring.
>
> Further, if I create a watch on something, *should* it be unmountable, just as
> if I had a file open there or had chdir'd into there?
I don't really know. I guess it depends on how it's being used? If
someone decides to e.g. make a file browser that installs watches for
a bunch of mountpoints for some fancy sidebar showing the device
mounts on the system, or something like that, that probably shouldn't
inhibit unmounting... I don't know if that's a realistic use case.
^ permalink raw reply
* Re: [RFC][PATCH 0/7] Mount, FS, Block and Keyrings notifications
From: Greg KH @ 2019-05-28 23:58 UTC (permalink / raw)
To: David Howells
Cc: viro, raven, linux-fsdevel, linux-api, linux-block, keyrings,
linux-security-module, linux-kernel
In-Reply-To: <155905930702.7587.7100265859075976147.stgit@warthog.procyon.org.uk>
On Tue, May 28, 2019 at 05:01:47PM +0100, David Howells wrote:
> Things I want to avoid:
>
> (1) Introducing features that make the core VFS dependent on the network
> stack or networking namespaces (ie. usage of netlink).
>
> (2) Dumping all this stuff into dmesg and having a daemon that sits there
> parsing the output and distributing it as this then puts the
> responsibility for security into userspace and makes handling
> namespaces tricky. Further, dmesg might not exist or might be
> inaccessible inside a container.
>
> (3) Letting users see events they shouldn't be able to see.
How are you handling namespaces then? Are they determined by the
namespace of the process that opened the original device handle, or the
namespace that made the new syscall for the events to "start flowing"?
Am I missing the logic that determines this in the patches, or is that
not implemented yet?
thanks,
greg k-h
^ permalink raw reply
* Fwd: [PATCH 2/2] ceph: add selinux support
From: Yan, Zheng @ 2019-05-29 3:38 UTC (permalink / raw)
To: LSM List
In-Reply-To: <20190527110702.3962-2-zyan@redhat.com>
---------- Forwarded message ---------
From: Yan, Zheng <zyan@redhat.com>
Date: Mon, May 27, 2019 at 7:11 PM
Subject: [PATCH 2/2] ceph: add selinux support
To: <ceph-devel@vger.kernel.org>
Cc: <idryomov@redhat.com>, <jlayton@redhat.com>, Yan, Zheng <zyan@redhat.com>
When creating new file/directory, uses dentry_init_security() to prepare
selinux context for the new inode, then sends openc/mkdir request to MDS,
together with selinux xattr.
Signed-off-by: "Yan, Zheng" <zyan@redhat.com>
---
fs/ceph/Kconfig | 12 +++++
fs/ceph/caps.c | 1 +
fs/ceph/dir.c | 12 +++++
fs/ceph/file.c | 3 ++
fs/ceph/inode.c | 1 +
fs/ceph/super.h | 19 +++++++
fs/ceph/xattr.c | 141 ++++++++++++++++++++++++++++++++++++++++++------
7 files changed, 172 insertions(+), 17 deletions(-)
diff --git a/fs/ceph/Kconfig b/fs/ceph/Kconfig
index 52095f473464..5a665c126a7c 100644
--- a/fs/ceph/Kconfig
+++ b/fs/ceph/Kconfig
@@ -35,3 +35,15 @@ config CEPH_FS_POSIX_ACL
groups beyond the owner/group/world scheme.
If you don't know what Access Control Lists are, say N
+
+config CEPH_FS_SECURITY_LABEL
+ bool "CephFS Security Labels"
+ depends on CEPH_FS && SECURITY
+ help
+ Security labels support alternative access control models
+ implemented by security modules like SELinux. This option
+ enables an extended attribute handler for file security
+ labels in the Ceph filesystem.
+
+ If you are not using a security module that requires using
+ extended attributes for file security labels, say N.
diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
index 7754d7679122..50409d9fdc90 100644
--- a/fs/ceph/caps.c
+++ b/fs/ceph/caps.c
@@ -3156,6 +3156,7 @@ static void handle_cap_grant(struct inode *inode,
ci->i_xattrs.blob = ceph_buffer_get(xattr_buf);
ci->i_xattrs.version = version;
ceph_forget_all_cached_acls(inode);
+ ceph_security_invalidate_secctx(inode);
}
}
diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index 14d795e5fa73..b282d076dc9e 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -839,6 +839,9 @@ static int ceph_mknod(struct inode *dir, struct
dentry *dentry,
err = ceph_pre_init_acls(dir, &mode, &as_ctx);
if (err < 0)
goto out;
+ err = ceph_security_init_secctx(dentry, mode, &as_ctx);
+ if (err < 0)
+ goto out;
dout("mknod in dir %p dentry %p mode 0%ho rdev %d\n",
dir, dentry, mode, rdev);
@@ -884,6 +887,7 @@ static int ceph_symlink(struct inode *dir, struct
dentry *dentry,
struct ceph_fs_client *fsc = ceph_sb_to_client(dir->i_sb);
struct ceph_mds_client *mdsc = fsc->mdsc;
struct ceph_mds_request *req;
+ struct ceph_acl_sec_ctx as_ctx = {};
int err;
if (ceph_snap(dir) != CEPH_NOSNAP)
@@ -894,6 +898,10 @@ static int ceph_symlink(struct inode *dir, struct
dentry *dentry,
goto out;
}
+ err = ceph_security_init_secctx(dentry, S_IFLNK | S_IRWXUGO, &as_ctx);
+ if (err < 0)
+ goto out;
+
dout("symlink in dir %p dentry %p to '%s'\n", dir, dentry, dest);
req = ceph_mdsc_create_request(mdsc, CEPH_MDS_OP_SYMLINK, USE_AUTH_MDS);
if (IS_ERR(req)) {
@@ -919,6 +927,7 @@ static int ceph_symlink(struct inode *dir, struct
dentry *dentry,
out:
if (err)
d_drop(dentry);
+ ceph_release_acl_sec_ctx(&as_ctx);
return err;
}
@@ -953,6 +962,9 @@ static int ceph_mkdir(struct inode *dir, struct
dentry *dentry, umode_t mode)
err = ceph_pre_init_acls(dir, &mode, &as_ctx);
if (err < 0)
goto out;
+ err = ceph_security_init_secctx(dentry, mode, &as_ctx);
+ if (err < 0)
+ goto out;
req = ceph_mdsc_create_request(mdsc, op, USE_AUTH_MDS);
if (IS_ERR(req)) {
diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index 5975345753d7..a7080783fe20 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -453,6 +453,9 @@ int ceph_atomic_open(struct inode *dir, struct
dentry *dentry,
err = ceph_pre_init_acls(dir, &mode, &as_ctx);
if (err < 0)
return err;
+ err = ceph_security_init_secctx(dentry, mode, &as_ctx);
+ if (err < 0)
+ goto out_ctx;
}
/* do the open */
diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index 30d0cdc21035..125ac54b5841 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -891,6 +891,7 @@ static int fill_inode(struct inode *inode, struct
page *locked_page,
iinfo->xattr_data, iinfo->xattr_len);
ci->i_xattrs.version = le64_to_cpu(info->xattr_version);
ceph_forget_all_cached_acls(inode);
+ ceph_security_invalidate_secctx(inode);
xattr_blob = NULL;
}
diff --git a/fs/ceph/super.h b/fs/ceph/super.h
index d7520ccf27e9..9c82d213a5ab 100644
--- a/fs/ceph/super.h
+++ b/fs/ceph/super.h
@@ -932,6 +932,10 @@ struct ceph_acl_sec_ctx {
#ifdef CONFIG_CEPH_FS_POSIX_ACL
void *default_acl;
void *acl;
+#endif
+#ifdef CONFIG_CEPH_FS_SECURITY_LABEL
+ void *sec_ctx;
+ u32 sec_ctxlen;
#endif
struct ceph_pagelist *pagelist;
};
@@ -950,6 +954,21 @@ static inline bool
ceph_security_xattr_wanted(struct inode *in)
}
#endif
+#ifdef CONFIG_CEPH_FS_SECURITY_LABEL
+extern int ceph_security_init_secctx(struct dentry *dentry, umode_t mode,
+ struct ceph_acl_sec_ctx *ctx);
+extern void ceph_security_invalidate_secctx(struct inode *inode);
+#else
+static inline int ceph_security_init_secctx(struct dentry *dentry,
umode_t mode,
+ struct ceph_acl_sec_ctx *ctx)
+{
+ return 0;
+}
+static inline void ceph_security_invalidate_secctx(struct inode *inode)
+{
+}
+#endif
+
void ceph_release_acl_sec_ctx(struct ceph_acl_sec_ctx *as_ctx);
/* acl.c */
diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
index 518a5beed58c..fea70696f375 100644
--- a/fs/ceph/xattr.c
+++ b/fs/ceph/xattr.c
@@ -8,6 +8,7 @@
#include <linux/ceph/decode.h>
#include <linux/xattr.h>
+#include <linux/security.h>
#include <linux/posix_acl_xattr.h>
#include <linux/slab.h>
@@ -17,26 +18,9 @@
static int __remove_xattr(struct ceph_inode_info *ci,
struct ceph_inode_xattr *xattr);
-static const struct xattr_handler ceph_other_xattr_handler;
-
-/*
- * List of handlers for synthetic system.* attributes. Other
- * attributes are handled directly.
- */
-const struct xattr_handler *ceph_xattr_handlers[] = {
-#ifdef CONFIG_CEPH_FS_POSIX_ACL
- &posix_acl_access_xattr_handler,
- &posix_acl_default_xattr_handler,
-#endif
- &ceph_other_xattr_handler,
- NULL,
-};
-
static bool ceph_is_valid_xattr(const char *name)
{
return !strncmp(name, XATTR_CEPH_PREFIX, XATTR_CEPH_PREFIX_LEN) ||
- !strncmp(name, XATTR_SECURITY_PREFIX,
- XATTR_SECURITY_PREFIX_LEN) ||
!strncmp(name, XATTR_TRUSTED_PREFIX, XATTR_TRUSTED_PREFIX_LEN) ||
!strncmp(name, XATTR_USER_PREFIX, XATTR_USER_PREFIX_LEN);
}
@@ -1196,6 +1180,110 @@ bool ceph_security_xattr_deadlock(struct inode *in)
spin_unlock(&ci->i_ceph_lock);
return ret;
}
+
+#ifdef CONFIG_CEPH_FS_SECURITY_LABEL
+int ceph_security_init_secctx(struct dentry *dentry, umode_t mode,
+ struct ceph_acl_sec_ctx *as_ctx)
+{
+ struct ceph_pagelist *pagelist = as_ctx->pagelist;
+ const char *name;
+ size_t name_len;
+ int err;
+
+ err = security_dentry_init_security(dentry, mode, &dentry->d_name,
+ &as_ctx->sec_ctx,
+ &as_ctx->sec_ctxlen);
+ if (err < 0) {
+ err = 0; /* do nothing */
+ goto out;
+ }
+
+ err = -ENOMEM;
+ if (!pagelist) {
+ pagelist = ceph_pagelist_alloc(GFP_KERNEL);
+ if (!pagelist)
+ goto out;
+ err = ceph_pagelist_reserve(pagelist, PAGE_SIZE);
+ if (err)
+ goto out;
+ ceph_pagelist_encode_32(pagelist, 1);
+ }
+
+ /*
+ * FIXME: Make security_dentry_init_security() generic. Currently
+ * It only supports single security module and only selinux has
+ * dentry_init_security hook.
+ */
+ name = XATTR_NAME_SELINUX;
+ name_len = strlen(name);
+ err = ceph_pagelist_reserve(pagelist,
+ 4 * 2 + name_len + as_ctx->sec_ctxlen);
+ if (err)
+ goto out;
+
+ if (as_ctx->pagelist) {
+ /* update count of KV pairs */
+ BUG_ON(pagelist->length <= sizeof(__le32));
+ if (list_is_singular(&pagelist->head)) {
+ le32_add_cpu((__le32*)pagelist->mapped_tail, 1);
+ } else {
+ struct page *page = list_first_entry(&pagelist->head,
+ struct page, lru);
+ void *addr = kmap_atomic(page);
+ le32_add_cpu((__le32*)addr, 1);
+ kunmap_atomic(addr);
+ }
+ } else {
+ as_ctx->pagelist = pagelist;
+ }
+
+ ceph_pagelist_encode_32(pagelist, name_len);
+ ceph_pagelist_append(pagelist, name, name_len);
+
+ ceph_pagelist_encode_32(pagelist, as_ctx->sec_ctxlen);
+ ceph_pagelist_append(pagelist, as_ctx->sec_ctx, as_ctx->sec_ctxlen);
+
+ err = 0;
+out:
+ if (pagelist && !as_ctx->pagelist)
+ ceph_pagelist_release(pagelist);
+ return err;
+}
+
+void ceph_security_invalidate_secctx(struct inode *inode)
+{
+ security_inode_invalidate_secctx(inode);
+}
+
+static int ceph_xattr_set_security_label(const struct xattr_handler *handler,
+ struct dentry *unused, struct inode *inode,
+ const char *key, const void *buf,
+ size_t buflen, int flags)
+{
+ if (security_ismaclabel(key)) {
+ const char *name = xattr_full_name(handler, key);
+ return __ceph_setxattr(inode, name, buf, buflen, flags);
+ }
+ return -EOPNOTSUPP;
+}
+
+static int ceph_xattr_get_security_label(const struct xattr_handler *handler,
+ struct dentry *unused, struct inode *inode,
+ const char *key, void *buf, size_t buflen)
+{
+ if (security_ismaclabel(key)) {
+ const char *name = xattr_full_name(handler, key);
+ return __ceph_getxattr(inode, name, buf, buflen);
+ }
+ return -EOPNOTSUPP;
+}
+
+static const struct xattr_handler ceph_security_label_handler = {
+ .prefix = XATTR_SECURITY_PREFIX,
+ .get = ceph_xattr_get_security_label,
+ .set = ceph_xattr_set_security_label,
+};
+#endif
#endif
void ceph_release_acl_sec_ctx(struct ceph_acl_sec_ctx *as_ctx)
@@ -1203,7 +1291,26 @@ void ceph_release_acl_sec_ctx(struct
ceph_acl_sec_ctx *as_ctx)
#ifdef CONFIG_CEPH_FS_POSIX_ACL
posix_acl_release(as_ctx->acl);
posix_acl_release(as_ctx->default_acl);
+#endif
+#ifdef CONFIG_CEPH_FS_SECURITY_LABEL
+ security_release_secctx(as_ctx->sec_ctx, as_ctx->sec_ctxlen);
#endif
if (as_ctx->pagelist)
ceph_pagelist_release(as_ctx->pagelist);
}
+
+/*
+ * List of handlers for synthetic system.* attributes. Other
+ * attributes are handled directly.
+ */
+const struct xattr_handler *ceph_xattr_handlers[] = {
+#ifdef CONFIG_CEPH_FS_POSIX_ACL
+ &posix_acl_access_xattr_handler,
+ &posix_acl_default_xattr_handler,
+#endif
+#ifdef CONFIG_CEPH_FS_SECURITY_LABEL
+ &ceph_security_label_handler,
+#endif
+ &ceph_other_xattr_handler,
+ NULL,
+};
--
2.17.2
^ permalink raw reply related
* Re: [RFC][PATCH 0/7] Mount, FS, Block and Keyrings notifications
From: Amir Goldstein @ 2019-05-29 6:33 UTC (permalink / raw)
To: David Howells
Cc: Al Viro, Ian Kent, linux-fsdevel, linux-api, linux-block,
keyrings, LSM List, linux-kernel, Jan Kara
In-Reply-To: <155905930702.7587.7100265859075976147.stgit@warthog.procyon.org.uk>
On Tue, May 28, 2019 at 7:03 PM David Howells <dhowells@redhat.com> wrote:
>
>
> Hi Al,
>
> Here's a set of patches to add a general variable-length notification queue
> concept and to add sources of events for:
>
> (1) Mount topology events, such as mounting, unmounting, mount expiry,
> mount reconfiguration.
>
> (2) Superblock events, such as R/W<->R/O changes, quota overrun and I/O
> errors (not complete yet).
>
> (3) Block layer events, such as I/O errors.
>
> (4) Key/keyring events, such as creating, linking and removal of keys.
>
> One of the reasons for this is so that we can remove the issue of processes
> having to repeatedly and regularly scan /proc/mounts, which has proven to
> be a system performance problem. To further aid this, the fsinfo() syscall
> on which this patch series depends, provides a way to access superblock and
> mount information in binary form without the need to parse /proc/mounts.
>
>
> Design decisions:
>
> (1) A misc chardev is used to create and open a ring buffer:
>
> fd = open("/dev/watch_queue", O_RDWR);
>
> which is then configured and mmap'd into userspace:
>
> ioctl(fd, IOC_WATCH_QUEUE_SET_SIZE, BUF_SIZE);
> ioctl(fd, IOC_WATCH_QUEUE_SET_FILTER, &filter);
> buf = mmap(NULL, BUF_SIZE * page_size, PROT_READ | PROT_WRITE,
> MAP_SHARED, fd, 0);
>
> The fd cannot be read or written (though there is a facility to use
> write to inject records for debugging) and userspace just pulls data
> directly out of the buffer.
>
> (2) The ring index pointers are stored inside the ring and are thus
> accessible to userspace. Userspace should only update the tail
> pointer and never the head pointer or risk breaking the buffer. The
> kernel checks that the pointers appear valid before trying to use
> them. A 'skip' record is maintained around the pointers.
>
> (3) poll() can be used to wait for data to appear in the buffer.
>
> (4) Records in the buffer are binary, typed and have a length so that they
> can be of varying size.
>
> This means that multiple heterogeneous sources can share a common
> buffer. Tags may be specified when a watchpoint is created to help
> distinguish the sources.
>
> (5) The queue is reusable as there are 16 million types available, of
> which I've used 4, so there is scope for others to be used.
>
> (6) Records are filterable as types have up to 256 subtypes that can be
> individually filtered. Other filtration is also available.
>
> (7) Each time the buffer is opened, a new buffer is created - this means
> that there's no interference between watchers.
>
> (8) When recording a notification, the kernel will not sleep, but will
> rather mark a queue as overrun if there's insufficient space, thereby
> avoiding userspace causing the kernel to hang.
>
> (9) The 'watchpoint' should be specific where possible, meaning that you
> specify the object that you want to watch.
>
> (10) The buffer is created and then watchpoints are attached to it, using
> one of:
>
> keyctl_watch_key(KEY_SPEC_SESSION_KEYRING, fd, 0x01);
> mount_notify(AT_FDCWD, "/", 0, fd, 0x02);
> sb_notify(AT_FDCWD, "/mnt", 0, fd, 0x03);
>
> where in all three cases, fd indicates the queue and the number after
> is a tag between 0 and 255.
>
> (11) The watch must be removed if either the watch buffer is destroyed or
> the watched object is destroyed.
>
>
> Things I want to avoid:
>
> (1) Introducing features that make the core VFS dependent on the network
> stack or networking namespaces (ie. usage of netlink).
>
> (2) Dumping all this stuff into dmesg and having a daemon that sits there
> parsing the output and distributing it as this then puts the
> responsibility for security into userspace and makes handling
> namespaces tricky. Further, dmesg might not exist or might be
> inaccessible inside a container.
>
> (3) Letting users see events they shouldn't be able to see.
>
>
> Further things that could be considered:
>
> (1) Adding a keyctl call to allow a watch on a keyring to be extended to
> "children" of that keyring, such that the watch is removed from the
> child if it is unlinked from the keyring.
>
> (2) Adding global superblock event queue.
>
> (3) Propagating watches to child superblock over automounts.
>
David,
I am interested to know how you envision filesystem notifications would
look with this interface.
fanotify can certainly benefit from providing a ring buffer interface to read
events.
From what I have seen, a common practice of users is to monitor mounts
(somehow) and place FAN_MARK_MOUNT fanotify watches dynamically.
It'd be good if those users can use a single watch mechanism/API for
watching the mount namespace and filesystem events within mounts.
A similar usability concern is with sb_notify and FAN_MARK_FILESYSTEM.
It provides users with two complete different mechanisms to watch error
and filesystem events. That is generally not a good thing to have.
I am not asking that you implement fs_notify() before merging sb_notify()
and I understand that you have a use case for sb_notify().
I am asking that you show me the path towards a unified API (how a
typical program would look like), so that we know before merging your
new API that it could be extended to accommodate fsnotify events
where the final result will look wholesome to users.
Thanks,
Amir.
^ permalink raw reply
* Re: [RFC][PATCH 0/7] Mount, FS, Block and Keyrings notifications
From: David Howells @ 2019-05-29 6:45 UTC (permalink / raw)
To: Amir Goldstein
Cc: dhowells, Al Viro, Ian Kent, linux-fsdevel, linux-api,
linux-block, keyrings, LSM List, linux-kernel, Jan Kara
In-Reply-To: <CAOQ4uxjC1M7jwjd9zSaSa6UW2dbEjc+ZbFSo7j9F1YHAQxQ8LQ@mail.gmail.com>
Amir Goldstein <amir73il@gmail.com> wrote:
> I am interested to know how you envision filesystem notifications would
> look with this interface.
What sort of events are you thinking of by "filesystem notifications"? You
mean things like file changes?
David
^ permalink raw reply
* Re: [RFC][PATCH 0/7] Mount, FS, Block and Keyrings notifications
From: Amir Goldstein @ 2019-05-29 7:40 UTC (permalink / raw)
To: David Howells
Cc: Al Viro, Ian Kent, linux-fsdevel, linux-api, linux-block,
keyrings, LSM List, linux-kernel, Jan Kara
In-Reply-To: <22971.1559112346@warthog.procyon.org.uk>
On Wed, May 29, 2019 at 9:45 AM David Howells <dhowells@redhat.com> wrote:
>
> Amir Goldstein <amir73il@gmail.com> wrote:
>
> > I am interested to know how you envision filesystem notifications would
> > look with this interface.
>
> What sort of events are you thinking of by "filesystem notifications"? You
> mean things like file changes?
I mean all the events provided by
http://man7.org/linux/man-pages/man7/fanotify.7.html
Which was recently (v4.20) extended to support watching a super block
and more recently (v5.1) extended to support watching directory entry
modifications.
Thanks,
Amir.
^ permalink raw reply
* Re: [RFC][PATCH 0/7] Mount, FS, Block and Keyrings notifications
From: David Howells @ 2019-05-29 9:09 UTC (permalink / raw)
To: Greg KH, casey
Cc: dhowells, viro, raven, linux-fsdevel, linux-api, linux-block,
keyrings, linux-security-module, linux-kernel
In-Reply-To: <20190528235810.GA5776@kroah.com>
Greg KH <gregkh@linuxfoundation.org> wrote:
> > (3) Letting users see events they shouldn't be able to see.
>
> How are you handling namespaces then? Are they determined by the
> namespace of the process that opened the original device handle, or the
> namespace that made the new syscall for the events to "start flowing"?
So far I haven't had to deal directly with namespaces.
mount_notify() requires you to have access to the mountpoint you want to watch
- and the entire tree rooted there is in one namespace, so your event sources
are restricted to that namespace. Further, mount objects don't themselves
have any other namespaces, not even a user_ns.
sb_notify() requires you to have access to the superblock you want to watch.
superblocks aren't directly namespaced as a class, though individual
superblocks may participate in particular namespaces (ipc, net, etc.). I'm
thinking some of these should be marked unwatchable (all pseudo superblocks,
kernfs-class, proc, for example).
Superblocks, however, do each have a user_ns - but you were allowed to access
the superblock by pathwalk, so you must have some access to the user_ns - I
think.
KEYCTL_NOTIFY requires you to have View access on the key you're watching.
Currently, keys have no real namespace restrictions, though I have patches to
include a namespace tag in the lookup criteria.
block_notify() doesn't require any direct access since you're watching a
global queue and there is no blockdev namespacing. LSMs are given the option
to filter events, though. The thought here is that if you can access dmesg,
you should be able to watch for blockdev events.
Actually, thinking further on this, restricting access to events is trickier
than I thought and than perhaps Casey was suggesting.
Say you're watching a mount object and someone in a different user_ns
namespace or with a different security label mounts on it. What governs
whether you are allowed to see the event?
You're watching the object for changes - and it *has* changed. Further, you
might be able to see the result of this change by other means (/proc/mounts,
for instance).
Should you be denied the event based on the security model?
On the other hand, if you're watching a tree of mount objects, it could be
argued that you should be denied access to events on any mount object you
can't reach by pathwalk.
On the third hand, if you can see it in /proc/mounts or by fsinfo(), you
should get an event for it.
> How are you handling namespaces then?
So to go back to the original question. At the moment they haven't impinged
directly and I haven't had to deal with them directly. There are indirect
namespace restrictions that I get for free just due to pathwalk, for instance.
David
^ permalink raw reply
* Re: [PATCH 3/7] vfs: Add a mount-notification facility
From: David Howells @ 2019-05-29 10:55 UTC (permalink / raw)
To: Jann Horn
Cc: dhowells, Al Viro, raven, linux-fsdevel, Linux API, linux-block,
keyrings, linux-security-module, kernel list
In-Reply-To: <CAG48ez2rRh2_Kq_EGJs5k-ZBNffGs_Q=vkQdinorBgo58tbGpg@mail.gmail.com>
Jann Horn <jannh@google.com> wrote:
> > + /* Global root? */
> > + if (mnt != parent) {
> > + cursor.dentry = READ_ONCE(mnt->mnt_mountpoint);
> > + mnt = parent;
> > + cursor.mnt = &mnt->mnt;
> > + continue;
> > + }
> > + break;
>
> (nit: this would look clearer if you inverted the condition and wrote
> it as "if (mnt == parent) break;", then you also wouldn't need that
> "continue" or the braces)
It does look better with the logic inverted, but you *do* still need the
continue. After the if-statement, there is:
cursor.dentry = cursor.dentry->d_parent;
which we need to skip. It might make sense to move that into an
else-statement from an aesthetic point of view.
David
^ 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