Hi Any, Jann, On Wed, Oct 03, 2018 at 03:08:12PM -0700, Andy Lutomirski wrote: > On Tue, Oct 2, 2018 at 12:36 PM Jann Horn <ja...@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 <j...@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.ker...@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.
I have read CVE-2013-1959... Thanks for Jann and Andy's review. I will create the sysfs interface through other way, then using file_ns_capable() for capability checking in next version. Thanks a lot! Joey Lee