ceph: set sec_context xattr on symlink creation
diff mbox series

Message ID 20200728191838.315530-1-jlayton@kernel.org
State New
Headers show
Series
  • ceph: set sec_context xattr on symlink creation
Related show

Commit Message

Jeff Layton July 28, 2020, 7:18 p.m. UTC
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(+)

Comments

Ilya Dryomov Aug. 3, 2020, 9:33 a.m. UTC | #1
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
Jeff Layton Aug. 3, 2020, 10:41 a.m. UTC | #2
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 Aug. 4, 2020, 12:29 p.m. UTC | #3
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,
Ilya Dryomov Aug. 4, 2020, 1:51 p.m. UTC | #4
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

Patch
diff mbox series

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