Re: [PATCH v2] RFC: fuse: Call security hooks on new inodes

[Chirantan Ekbote <chirantan@chromium.org>]

On Tue, Jun 16, 2020 at 6:29 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Wed, Jun 10, 2020 at 11:27 AM Chirantan Ekbote
> <chirantan@chromium.org> wrote:
> >
> >
> > When set to true, get the security context for a newly created inode via
> > `security_dentry_init_security` and append it to the create, mkdir,
> > mknod, and symlink requests.  The server should use this context by
> > writing it to `/proc/thread-self/attr/fscreate` before creating the
> > requested inode.
>
> This is confusing.  You mean if the server is stacking on top of a
> real fs, then it can force the created new inode to have the given
> security attributes by writing to that proc file?
>

Yes that's correct.  Writing to that proc file ends up setting a field
in an selinux struct in the kernel.  Later, when an inode is created
the selinux security hook uses that field to determine the label that
should be applied to the inode.  This ensures that inodes appear
atomically with the correct selinux labels.  Most users actually end
up using setfscreatecon from libselinux but all that does is write to
/proc/thread-self/attr/fscreate itself after doing some
conversion/validation.

> >
> >  static void fuse_advise_use_readdirplus(struct inode *dir)
> >  {
> > @@ -442,6 +445,8 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> >         struct fuse_entry_out outentry;
> >         struct fuse_inode *fi;
> >         struct fuse_file *ff;
> > +       void *security_ctx = NULL;
> > +       u32 security_ctxlen = 0;
> >
> >         /* Userspace expects S_IFREG in create mode */
> >         BUG_ON((mode & S_IFMT) != S_IFREG);
> > @@ -477,6 +482,21 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> >         args.out_args[0].value = &outentry;
> >         args.out_args[1].size = sizeof(outopen);
> >         args.out_args[1].value = &outopen;
> > +
> > +       if (fc->init_security) {
> > +               err = security_dentry_init_security(entry, mode, &entry->d_name,
> > +                                                   &security_ctx,
> > +                                                   &security_ctxlen);
> > +               if (err)
> > +                       goto out_put_forget_req;
> > +
> > +               if (security_ctxlen > 0) {
> > +                       args.in_numargs = 3;
> > +                       args.in_args[2].size = security_ctxlen;
> > +                       args.in_args[2].value = security_ctx;
> > +               }
> > +       }
> > +
>
> The above is quadruplicated, a helper is in order.

Ack.

>
> >         err = fuse_simple_request(fc, &args);
> >         if (err)
> >                 goto out_free_ff;
> > @@ -513,6 +533,8 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> >         return err;
> >
> >  out_free_ff:
> > +       if (security_ctxlen > 0)
> > +               kfree(security_ctx);
>
> Freeing NULL is okay, if that's guaranteed in case of security_ctxlen
> == 0, then you need not check that condition.

Ack.  Will fix in v3.