[Virtio-fs] [PULL 09/12] virtiofsd: Create new file with security context

[Vivek Goyal]

On Thu, Apr 07, 2022 at 01:44:35PM +0100, Dr. David Alan Gilbert wrote:
> * Peter Maydell (peter.maydell at linaro.org) wrote:
> > On Thu, 17 Feb 2022 at 17:40, Dr. David Alan Gilbert (git)
> > <dgilbert at redhat.com> wrote:
> > >
> > > From: Vivek Goyal <vgoyal at redhat.com>
> > >
> > > This patch adds support for creating new file with security context
> > > as sent by client. It basically takes three paths.
> > >
> > > - If no security context enabled, then it continues to create files without
> > >   security context.
> > >
> > > - If security context is enabled and but security.selinux has not been
> > >   remapped, then it uses /proc/thread-self/attr/fscreate knob to set
> > >   security context and then create the file. This will make sure that
> > >   newly created file gets the security context as set in "fscreate" and
> > >   this is atomic w.r.t file creation.
> > >
> > >   This is useful and host and guest SELinux policies don't conflict and
> > >   can work with each other. In that case, guest security.selinux xattr
> > >   is not remapped and it is passthrough as "security.selinux" xattr
> > >   on host.
> > >
> > > - If security context is enabled but security.selinux xattr has been
> > >   remapped to something else, then it first creates the file and then
> > >   uses setxattr() to set the remapped xattr with the security context.
> > >   This is a non-atomic operation w.r.t file creation.
> > >
> > >   This mode will be most versatile and allow host and guest to have their
> > >   own separate SELinux xattrs and have their own separate SELinux policies.
> > >
> > > Reviewed-by: Dr. David Alan Gilbert <dgilbert at redhat.com>
> > > Signed-off-by: Vivek Goyal <vgoyal at redhat.com>
> > > Message-Id: <20220208204813.682906-9-vgoyal at redhat.com>
> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert at redhat.com>
> > 
> > Hi; Coverity reports some issues (CID 1487142, 1487195), because
> > it is not a fan of the error-handling pattern used in this code:
> > 
> > > +static int do_mknod_symlink_secctx(fuse_req_t req, struct lo_inode *dir,
> > > +                                   const char *name, const char *secctx_name)
> > > +{
> > > +    int path_fd, err;
> > > +    char procname[64];
> > > +    struct lo_data *lo = lo_data(req);
> > > +
> > > +    if (!req->secctx.ctxlen) {
> > > +        return 0;
> > > +    }
> > > +
> > > +    /* Open newly created element with O_PATH */
> > > +    path_fd = openat(dir->fd, name, O_PATH | O_NOFOLLOW);
> > > +    err = path_fd == -1 ? errno : 0;
> > > +    if (err) {
> > > +        return err;
> > > +    }
> > 
> > We set err based on whether path_fd is -1 or not, but we decide
> > whether to early-return based on the value of err. Coverity
> > doesn't know that openat() will always set errno to something
> > non-zero if it returns -1, so it complains because it thinks
> > there's a code path where openat() returns -1, but errno is 0,
> > and so we don't take the early-return and instead continue
> > through all the code below to the "close(path_fd)", which
> > should not be being passed a negative value for the filedescriptor.
> > 
> > I could just mark these as false-positives, but it does seem a bit
> > odd that we are using two different conditions here. Perhaps it would
> > be better to rephrase? For instance, for the openat() we could write:
> > 
> >    path_fd = openat(dir->fd, name, O_PATH | O_NOFOLLOW);
> >    if (path_fd == -1) {
> >        return errno;
> >    }
> 
> That looks OK to me; please send a patch.
> 
> Some of the cases look like they need to just be a little careful that
> 'err' always gets set to 0 if there are later cases that might set err.

I think use of "err" to save errno pattern is used because in some
cases we can't return immediately after error. Instead we have to
take some actions to restore some state and then return.

So for this specific case, it looks fine because we don't have to
restore any state before returning.

Vivek
> 
> Dave
> 
> > and similarly for the openat() in open_set_proc_fscreate().
> > 
> > > +    sprintf(procname, "%i", path_fd);
> > > +    FCHDIR_NOFAIL(lo->proc_self_fd);
> > > +    /* Set security context. This is not atomic w.r.t file creation */
> > > +    err = setxattr(procname, secctx_name, req->secctx.ctx, req->secctx.ctxlen,
> > > +                   0);
> > > +    if (err) {
> > > +        err = errno;
> > > +    }
> > 
> > > +    FCHDIR_NOFAIL(lo->root.fd);
> > > +    close(path_fd);
> > > +    return err;
> > > +}
> > 
> > thanks
> > -- PMM
> > 
> -- 
> Dr. David Alan Gilbert / dgilbert at redhat.com / Manchester, UK
>