[PATCH] ceph: set sec_context xattr on symlink creation

[PATCH] ceph: set sec_context xattr on symlink creation

[Jeff Layton]

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);
-- 
2.26.2

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

[Ilya Dryomov]

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?

Thanks,

                Ilya

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

[Jeff Layton]

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.
-- 
Jeff Layton <jlayton@kernel.org>

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

[Jeff Layton]

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?

Thanks,
-- 
Jeff Layton <jlayton@kernel.org>

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

[Ilya Dryomov]

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