| Message ID | 20190626192234.11725-19-casey@schaufler-ca.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | LSM: Module stacking for AppArmor | expand |
On Wed, Jun 26, 2019 at 12:22:29PM -0700, Casey Schaufler wrote: > Change the security_dentry_init_security() interface to > fill an lsmcontext structure instead of a void * data area > and a length. The lone caller of this interface is NFS4, > which may make copies of the data using its own mechanisms. > A rework of the nfs4 code to use the lsmcontext properly > is a significant project, so the coward's way out is taken, > and the lsmcontext data from security_dentry_init_security() > is copied, then released directly. > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> Reviewed-by: Kees Cook <keescook@chromium.org> -Kees > --- > fs/nfs/nfs4proc.c | 26 ++++++++++++++++---------- > include/linux/security.h | 7 +++---- > security/security.c | 19 +++++++++++++++---- > 3 files changed, 34 insertions(+), 18 deletions(-) > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > index af1c0db29c39..952f805965bb 100644 > --- a/fs/nfs/nfs4proc.c > +++ b/fs/nfs/nfs4proc.c > @@ -113,6 +113,7 @@ static inline struct nfs4_label * > nfs4_label_init_security(struct inode *dir, struct dentry *dentry, > struct iattr *sattr, struct nfs4_label *label) > { > + struct lsmcontext context; > int err; > > if (label == NULL) > @@ -122,21 +123,26 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, > return NULL; > > err = security_dentry_init_security(dentry, sattr->ia_mode, > - &dentry->d_name, (void **)&label->label, &label->len); > - if (err == 0) > - return label; > + &dentry->d_name, &context); > + > + if (err) > + return NULL; > + > + label->label = kmemdup(context.context, context.len, GFP_KERNEL); > + if (label->label == NULL) > + label = NULL; > + else > + label->len = context.len; > + > + security_release_secctx(&context); > + > + return label; > > - return NULL; > } > static inline void > nfs4_label_release_security(struct nfs4_label *label) > { > - struct lsmcontext scaff; /* scaffolding */ > - > - if (label) { > - lsmcontext_init(&scaff, label->label, label->len, 0); > - security_release_secctx(&scaff); > - } > + kfree(label->label); > } > static inline u32 *nfs4_bitmask(struct nfs_server *server, struct nfs4_label *label) > { > diff --git a/include/linux/security.h b/include/linux/security.h > index 7255825aa697..2674eb70c2d7 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -352,8 +352,8 @@ int security_sb_clone_mnt_opts(const struct super_block *oldsb, > int security_add_mnt_opt(const char *option, const char *val, > int len, void **mnt_opts); > int security_dentry_init_security(struct dentry *dentry, int mode, > - const struct qstr *name, void **ctx, > - u32 *ctxlen); > + const struct qstr *name, > + struct lsmcontext *ctx); > int security_dentry_create_files_as(struct dentry *dentry, int mode, > struct qstr *name, > const struct cred *old, > @@ -724,8 +724,7 @@ static inline void security_inode_free(struct inode *inode) > static inline int security_dentry_init_security(struct dentry *dentry, > int mode, > const struct qstr *name, > - void **ctx, > - u32 *ctxlen) > + struct lsmcontext *ctx) > { > return -EOPNOTSUPP; > } > diff --git a/security/security.c b/security/security.c > index 97b468f6e6a9..61cdc6bcd32e 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -1024,11 +1024,22 @@ void security_inode_free(struct inode *inode) > } > > int security_dentry_init_security(struct dentry *dentry, int mode, > - const struct qstr *name, void **ctx, > - u32 *ctxlen) > + const struct qstr *name, > + struct lsmcontext *cp) > { > - return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode, > - name, ctx, ctxlen); > + int *display = current->security; > + struct security_hook_list *hp; > + > + hlist_for_each_entry(hp, &security_hook_heads.dentry_init_security, > + list) > + if (*display == LSMBLOB_INVALID || > + *display == hp->lsmid->slot) { > + cp->slot = hp->lsmid->slot; > + return hp->hook.dentry_init_security(dentry, mode, > + name, (void **)&cp->context, &cp->len); > + } > + > + return -EOPNOTSUPP; > } > EXPORT_SYMBOL(security_dentry_init_security); > > -- > 2.20.1 >
On 6/26/19 12:22 PM, Casey Schaufler wrote: > Change the security_dentry_init_security() interface to > fill an lsmcontext structure instead of a void * data area > and a length. The lone caller of this interface is NFS4, > which may make copies of the data using its own mechanisms. > A rework of the nfs4 code to use the lsmcontext properly > is a significant project, so the coward's way out is taken, > and the lsmcontext data from security_dentry_init_security() > is copied, then released directly. > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> Reviewed-by: John Johansen <john.johansen@canonical.com> > --- > fs/nfs/nfs4proc.c | 26 ++++++++++++++++---------- > include/linux/security.h | 7 +++---- > security/security.c | 19 +++++++++++++++---- > 3 files changed, 34 insertions(+), 18 deletions(-) > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > index af1c0db29c39..952f805965bb 100644 > --- a/fs/nfs/nfs4proc.c > +++ b/fs/nfs/nfs4proc.c > @@ -113,6 +113,7 @@ static inline struct nfs4_label * > nfs4_label_init_security(struct inode *dir, struct dentry *dentry, > struct iattr *sattr, struct nfs4_label *label) > { > + struct lsmcontext context; > int err; > > if (label == NULL) > @@ -122,21 +123,26 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, > return NULL; > > err = security_dentry_init_security(dentry, sattr->ia_mode, > - &dentry->d_name, (void **)&label->label, &label->len); > - if (err == 0) > - return label; > + &dentry->d_name, &context); > + > + if (err) > + return NULL; > + > + label->label = kmemdup(context.context, context.len, GFP_KERNEL); > + if (label->label == NULL) > + label = NULL; > + else > + label->len = context.len; > + > + security_release_secctx(&context); > + > + return label; > > - return NULL; > } > static inline void > nfs4_label_release_security(struct nfs4_label *label) > { > - struct lsmcontext scaff; /* scaffolding */ > - > - if (label) { > - lsmcontext_init(&scaff, label->label, label->len, 0); > - security_release_secctx(&scaff); > - } > + kfree(label->label); > } > static inline u32 *nfs4_bitmask(struct nfs_server *server, struct nfs4_label *label) > { > diff --git a/include/linux/security.h b/include/linux/security.h > index 7255825aa697..2674eb70c2d7 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -352,8 +352,8 @@ int security_sb_clone_mnt_opts(const struct super_block *oldsb, > int security_add_mnt_opt(const char *option, const char *val, > int len, void **mnt_opts); > int security_dentry_init_security(struct dentry *dentry, int mode, > - const struct qstr *name, void **ctx, > - u32 *ctxlen); > + const struct qstr *name, > + struct lsmcontext *ctx); > int security_dentry_create_files_as(struct dentry *dentry, int mode, > struct qstr *name, > const struct cred *old, > @@ -724,8 +724,7 @@ static inline void security_inode_free(struct inode *inode) > static inline int security_dentry_init_security(struct dentry *dentry, > int mode, > const struct qstr *name, > - void **ctx, > - u32 *ctxlen) > + struct lsmcontext *ctx) > { > return -EOPNOTSUPP; > } > diff --git a/security/security.c b/security/security.c > index 97b468f6e6a9..61cdc6bcd32e 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -1024,11 +1024,22 @@ void security_inode_free(struct inode *inode) > } > > int security_dentry_init_security(struct dentry *dentry, int mode, > - const struct qstr *name, void **ctx, > - u32 *ctxlen) > + const struct qstr *name, > + struct lsmcontext *cp) > { > - return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode, > - name, ctx, ctxlen); > + int *display = current->security; > + struct security_hook_list *hp; > + > + hlist_for_each_entry(hp, &security_hook_heads.dentry_init_security, > + list) > + if (*display == LSMBLOB_INVALID || > + *display == hp->lsmid->slot) { > + cp->slot = hp->lsmid->slot; > + return hp->hook.dentry_init_security(dentry, mode, > + name, (void **)&cp->context, &cp->len); > + } > + > + return -EOPNOTSUPP; > } > EXPORT_SYMBOL(security_dentry_init_security); > >
On Wed, Jun 26, 2019 at 9:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > Change the security_dentry_init_security() interface to > fill an lsmcontext structure instead of a void * data area > and a length. The lone caller of this interface is NFS4, > which may make copies of the data using its own mechanisms. > A rework of the nfs4 code to use the lsmcontext properly > is a significant project, so the coward's way out is taken, > and the lsmcontext data from security_dentry_init_security() > is copied, then released directly. > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > --- > fs/nfs/nfs4proc.c | 26 ++++++++++++++++---------- > include/linux/security.h | 7 +++---- > security/security.c | 19 +++++++++++++++---- > 3 files changed, 34 insertions(+), 18 deletions(-) > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > index af1c0db29c39..952f805965bb 100644 > --- a/fs/nfs/nfs4proc.c > +++ b/fs/nfs/nfs4proc.c > @@ -113,6 +113,7 @@ static inline struct nfs4_label * > nfs4_label_init_security(struct inode *dir, struct dentry *dentry, > struct iattr *sattr, struct nfs4_label *label) > { > + struct lsmcontext context; > int err; > > if (label == NULL) > @@ -122,21 +123,26 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, > return NULL; > > err = security_dentry_init_security(dentry, sattr->ia_mode, > - &dentry->d_name, (void **)&label->label, &label->len); > - if (err == 0) > - return label; > + &dentry->d_name, &context); > + > + if (err) > + return NULL; > + > + label->label = kmemdup(context.context, context.len, GFP_KERNEL); > + if (label->label == NULL) > + label = NULL; > + else > + label->len = context.len; > + > + security_release_secctx(&context); > + > + return label; > > - return NULL; > } > static inline void > nfs4_label_release_security(struct nfs4_label *label) > { > - struct lsmcontext scaff; /* scaffolding */ > - > - if (label) { > - lsmcontext_init(&scaff, label->label, label->len, 0); > - security_release_secctx(&scaff); > - } > + kfree(label->label); > } > static inline u32 *nfs4_bitmask(struct nfs_server *server, struct nfs4_label *label) > { > diff --git a/include/linux/security.h b/include/linux/security.h > index 7255825aa697..2674eb70c2d7 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -352,8 +352,8 @@ int security_sb_clone_mnt_opts(const struct super_block *oldsb, > int security_add_mnt_opt(const char *option, const char *val, > int len, void **mnt_opts); > int security_dentry_init_security(struct dentry *dentry, int mode, > - const struct qstr *name, void **ctx, > - u32 *ctxlen); > + const struct qstr *name, > + struct lsmcontext *ctx); > int security_dentry_create_files_as(struct dentry *dentry, int mode, > struct qstr *name, > const struct cred *old, > @@ -724,8 +724,7 @@ static inline void security_inode_free(struct inode *inode) > static inline int security_dentry_init_security(struct dentry *dentry, > int mode, > const struct qstr *name, > - void **ctx, > - u32 *ctxlen) > + struct lsmcontext *ctx) > { > return -EOPNOTSUPP; > } > diff --git a/security/security.c b/security/security.c > index 97b468f6e6a9..61cdc6bcd32e 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -1024,11 +1024,22 @@ void security_inode_free(struct inode *inode) > } > > int security_dentry_init_security(struct dentry *dentry, int mode, > - const struct qstr *name, void **ctx, > - u32 *ctxlen) > + const struct qstr *name, > + struct lsmcontext *cp) > { > - return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode, > - name, ctx, ctxlen); > + int *display = current->security; > + struct security_hook_list *hp; > + > + hlist_for_each_entry(hp, &security_hook_heads.dentry_init_security, > + list) > + if (*display == LSMBLOB_INVALID || > + *display == hp->lsmid->slot) { > + cp->slot = hp->lsmid->slot; > + return hp->hook.dentry_init_security(dentry, mode, > + name, (void **)&cp->context, &cp->len); > + } Sorry for chiming in only now, but does it really make sense to select the per-task display LSM here? I'm not sure what exactly NFS does with the returned context, but if it sends it to the server (it looks like it does), wouldn't this mean that the server would potentially get context from different LSMs, depending on the setting of the task which is creating the file/directory? > + > + return -EOPNOTSUPP; > } > EXPORT_SYMBOL(security_dentry_init_security); > > -- > 2.20.1 > -- Ondrej Mosnacek <omosnace at redhat dot com> Software Engineer, Security Technologies Red Hat, Inc.
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index af1c0db29c39..952f805965bb 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -113,6 +113,7 @@ static inline struct nfs4_label * nfs4_label_init_security(struct inode *dir, struct dentry *dentry, struct iattr *sattr, struct nfs4_label *label) { + struct lsmcontext context; int err; if (label == NULL) @@ -122,21 +123,26 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, return NULL; err = security_dentry_init_security(dentry, sattr->ia_mode, - &dentry->d_name, (void **)&label->label, &label->len); - if (err == 0) - return label; + &dentry->d_name, &context); + + if (err) + return NULL; + + label->label = kmemdup(context.context, context.len, GFP_KERNEL); + if (label->label == NULL) + label = NULL; + else + label->len = context.len; + + security_release_secctx(&context); + + return label; - return NULL; } static inline void nfs4_label_release_security(struct nfs4_label *label) { - struct lsmcontext scaff; /* scaffolding */ - - if (label) { - lsmcontext_init(&scaff, label->label, label->len, 0); - security_release_secctx(&scaff); - } + kfree(label->label); } static inline u32 *nfs4_bitmask(struct nfs_server *server, struct nfs4_label *label) { diff --git a/include/linux/security.h b/include/linux/security.h index 7255825aa697..2674eb70c2d7 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -352,8 +352,8 @@ int security_sb_clone_mnt_opts(const struct super_block *oldsb, int security_add_mnt_opt(const char *option, const char *val, int len, void **mnt_opts); int security_dentry_init_security(struct dentry *dentry, int mode, - const struct qstr *name, void **ctx, - u32 *ctxlen); + const struct qstr *name, + struct lsmcontext *ctx); int security_dentry_create_files_as(struct dentry *dentry, int mode, struct qstr *name, const struct cred *old, @@ -724,8 +724,7 @@ static inline void security_inode_free(struct inode *inode) static inline int security_dentry_init_security(struct dentry *dentry, int mode, const struct qstr *name, - void **ctx, - u32 *ctxlen) + struct lsmcontext *ctx) { return -EOPNOTSUPP; } diff --git a/security/security.c b/security/security.c index 97b468f6e6a9..61cdc6bcd32e 100644 --- a/security/security.c +++ b/security/security.c @@ -1024,11 +1024,22 @@ void security_inode_free(struct inode *inode) } int security_dentry_init_security(struct dentry *dentry, int mode, - const struct qstr *name, void **ctx, - u32 *ctxlen) + const struct qstr *name, + struct lsmcontext *cp) { - return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode, - name, ctx, ctxlen); + int *display = current->security; + struct security_hook_list *hp; + + hlist_for_each_entry(hp, &security_hook_heads.dentry_init_security, + list) + if (*display == LSMBLOB_INVALID || + *display == hp->lsmid->slot) { + cp->slot = hp->lsmid->slot; + return hp->hook.dentry_init_security(dentry, mode, + name, (void **)&cp->context, &cp->len); + } + + return -EOPNOTSUPP; } EXPORT_SYMBOL(security_dentry_init_security);
Change the security_dentry_init_security() interface to fill an lsmcontext structure instead of a void * data area and a length. The lone caller of this interface is NFS4, which may make copies of the data using its own mechanisms. A rework of the nfs4 code to use the lsmcontext properly is a significant project, so the coward's way out is taken, and the lsmcontext data from security_dentry_init_security() is copied, then released directly. Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> --- fs/nfs/nfs4proc.c | 26 ++++++++++++++++---------- include/linux/security.h | 7 +++---- security/security.c | 19 +++++++++++++++---- 3 files changed, 34 insertions(+), 18 deletions(-)