On Thu, 17 Feb 2022 at 17:40, Dr. David Alan Gilbert (git)
<dgilb...@redhat.com> wrote:
>
> From: Vivek Goyal <vgo...@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 <dgilb...@redhat.com>
> Signed-off-by: Vivek Goyal <vgo...@redhat.com>
> Message-Id: <20220208204813.682906-9-vgo...@redhat.com>
> Signed-off-by: Dr. David Alan Gilbert <dgilb...@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;
}
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
