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

[Miklos Szeredi <miklos@szeredi.hu>]

On Tue, Jun 16, 2020 at 11:41 AM Chirantan Ekbote
<chirantan@chromium.org> wrote:
>
> 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.

 FUSE servers do not necessarily use a real filesystem as a backing
store (e.g. network filesystems), so you should clarify that in the
description.

>
> > >
> > >  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.