Re: [PATCH] ceph: set sec_context xattr on symlink creation

[Ilya Dryomov <idryomov@gmail.com>]

On Tue, Aug 4, 2020 at 2:29 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Mon, 2020-08-03 at 06:41 -0400, Jeff Layton wrote:
> > On Mon, 2020-08-03 at 11:33 +0200, Ilya Dryomov wrote:
> > > On Tue, Jul 28, 2020 at 10:04 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > > Symlink inodes should have the security context set in their xattrs on
> > > > creation. We already set the context on creation, but we don't attach
> > > > the pagelist. Make it do so.
> > > >
> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > ---
> > > >  fs/ceph/dir.c | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> > > > index 39f5311404b0..060bdcc5ce32 100644
> > > > --- a/fs/ceph/dir.c
> > > > +++ b/fs/ceph/dir.c
> > > > @@ -930,6 +930,10 @@ static int ceph_symlink(struct inode *dir, struct dentry *dentry,
> > > >         req->r_num_caps = 2;
> > > >         req->r_dentry_drop = CEPH_CAP_FILE_SHARED | CEPH_CAP_AUTH_EXCL;
> > > >         req->r_dentry_unless = CEPH_CAP_FILE_EXCL;
> > > > +       if (as_ctx.pagelist) {
> > > > +               req->r_pagelist = as_ctx.pagelist;
> > > > +               as_ctx.pagelist = NULL;
> > > > +       }
> > > >         err = ceph_mdsc_do_request(mdsc, dir, req);
> > > >         if (!err && !req->r_reply_info.head->is_dentry)
> > > >                 err = ceph_handle_notrace_create(dir, dentry);
> > >
> > > What is the side effect?  Should this go to stable?
> > >
> >
> > The effect is that symlink inodes don't get an SELinux context set on
> > them at creation, so they end up unlabeled instead of inheriting the
> > proper context. As to the severity, it really depends on what ends up
> > being unlabeled.
> >
> > It's probably harmless enough to put this into stable, but I only
> > noticed it by inspection, so I'm not sure it meets the "it must fix a
> > real bug that bothers people" criterion.
>
> After thinking about it some more, let's do go ahead and mark this for
> stable. While no one has complained about it, it's a subtle bug that
> could be problematic once people start populating cephfs trees with
> unlabeled symlinks. Better that we fix it early before SELinux support
> becomes even more widespread.
>
> Ilya, can you add the Cc: stable tag before you send a PR to Linus?

Sure, will do.

Thanks,

                Ilya