Messages in this thread | | | From | Andy Lutomirski <> | Date | Wed, 3 Oct 2018 15:08:12 -0700 | Subject | Re: [PATCH 1/5] PM / hibernate: Create snapshot keys handler |
| |
On Tue, Oct 2, 2018 at 12:36 PM Jann Horn <jannh@google.com> wrote: > > +Andy for opinions on things in write handlers > +Mimi Zohar as EVM maintainer > > On Tue, Oct 2, 2018 at 9:55 AM joeyli <jlee@suse.com> wrote: > > On Thu, Sep 13, 2018 at 04:31:18PM +0200, Jann Horn wrote: > > > On Thu, Sep 13, 2018 at 4:08 PM Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote: > > > > This patch adds a snapshot keys handler for using the key retention > > > > service api to create keys for snapshot image encryption and > > > > authentication. > > [...snip] > > > > +static ssize_t disk_kmk_store(struct kobject *kobj, struct kobj_attribute *attr, > > > > + const char *buf, size_t n) > > > > +{ > > > > + int error = 0; > > > > + char *p; > > > > + int len; > > > > + > > > > + if (!capable(CAP_SYS_ADMIN)) > > > > + return -EPERM; > > > > > > This is wrong, you can't use capable() in a write handler. You'd have > > > to use file_ns_capable(), and I think sysfs currently doesn't give you > > > a pointer to the struct file. > > > If you want to do this in a write handler, you'll have to either get > > > rid of this check or plumb through the cred struct pointer. > > > Alternatively, you could use some interface that doesn't go through a > > > write handler. > > > > > > > Thank you for point out this problem. > > > > Actually the evm_write_key() is the example for my code. The > > difference is that evm creates interface file on securityfs, but my > > implementation is on sysfs: > > > > security/integrity/evm/evm_secfs.c > > > > static ssize_t evm_write_key(struct file *file, const char __user *buf, > > size_t count, loff_t *ppos) > > { > > int i, ret; > > > > if (!capable(CAP_SYS_ADMIN) || (evm_initialized & EVM_SETUP)) > > return -EPERM;
Yeah, that's a bug.
> > ... > > > > On the other hand, the writing handler of /sys/power/wake_lock also > > uses capable() to check the CAP_BLOCK_SUSPEND capability: > > > > kernel/power/main.c > > static ssize_t wake_lock_store(struct kobject *kobj, > > struct kobj_attribute *attr, > > const char *buf, size_t n) > > { > > int error = pm_wake_lock(buf); > > return error ? error : n; > > } > > power_attr(wake_lock); > > > > kernel/power/wakelock.c > > int pm_wake_lock(const char *buf) > > { > > ... > > if (!capable(CAP_BLOCK_SUSPEND)) > > return -EPERM; > > ...
Also a bug.
> > > > > > So I confused for when can capable() be used in sysfs interface? Is > > capable() only allowed in reading handler? Why the writing handler > > of securityfs can use capable()? > > They can't, they're all wrong. :P All of these capable() checks in > write handlers have to be assumed to be ineffective. But it's not a > big deal because you still need UID 0 to access these files.
Why are there capability checks at all?
> > > > > +static int user_key_init(void) > > > > +{ > > > > + struct user_key_payload *ukp; > > > > + struct key *key; > > > > + int err = 0; > > > > + > > > > + pr_debug("%s\n", __func__); > > > > + > > > > + /* find out swsusp-key */ > > > > + key = request_key(&key_type_user, skey.key_name, NULL); > > > > > > request_key() looks at current's keyring. That shouldn't happen in a > > > write handler. > > > > > > > The evm_write_key() also uses request_key() but it's on securityfs. Should > > I move my sysfs interface to securityfs? > > I don't think you should be doing this in the context of any > filesystem. If EVM does that, EVM is doing it wrong.
EVM sounds buggy.
In general if you look at current *at all* in an implementation of write() *in any filesystem*, you are doing it wrong.
| |