lkml.org 
[lkml]   [2018]   [Oct]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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.

\
 
 \ /
  Last update: 2018-10-04 00:09    [W:0.135 / U:3.196 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site