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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On Thu, 17 Feb 2022 at 17:40, Dr. David Alan Gilbert (git)
> <dgilbert@redhat.com> wrote:
> >
> > From: Vivek Goyal <vgoyal@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@redhat.com>
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > Message-Id: <20220208204813.682906-9-vgoyal@redhat.com>
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@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.

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@redhat.com / Manchester, UK