| Message ID | 1467733854-6314-2-git-send-email-vgoyal@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi,
[auto build test ERROR on miklos-vfs/overlayfs-next]
[also build test ERROR on v4.7-rc6 next-20160705]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Vivek-Goyal/Overlayfs-SELinux-Support/20160706-000037
base: https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git overlayfs-next
config: i386-randconfig-s0-201627 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
fs/overlayfs/copy_up.c: In function 'ovl_copy_up_locked':
>> fs/overlayfs/copy_up.c:262:39: error: passing argument 2 of 'security_inode_copy_up' from incompatible pointer type [-Werror=incompatible-pointer-types]
err = security_inode_copy_up(dentry, &old_creds);
^
In file included from fs/overlayfs/copy_up.c:16:0:
include/linux/security.h:762:19: note: expected 'struct dentry *' but argument is of type 'const struct cred **'
static inline int security_inode_copy_up(struct dentry *src, struct dentry *dst)
^~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/security_inode_copy_up +262 fs/overlayfs/copy_up.c
256 upper = lookup_one_len(dentry->d_name.name, upperdir,
257 dentry->d_name.len);
258 err = PTR_ERR(upper);
259 if (IS_ERR(upper))
260 goto out1;
261
> 262 err = security_inode_copy_up(dentry, &old_creds);
263 if (err < 0)
264 goto out2;
265
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi,
[auto build test WARNING on miklos-vfs/overlayfs-next]
[also build test WARNING on v4.7-rc6 next-20160705]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Vivek-Goyal/Overlayfs-SELinux-Support/20160706-000037
base: https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git overlayfs-next
config: m68k-sun3_defconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 4.9.0
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=m68k
All warnings (new ones prefixed by >>):
fs/overlayfs/copy_up.c: In function 'ovl_copy_up_locked':
>> fs/overlayfs/copy_up.c:262:8: warning: passing argument 2 of 'security_inode_copy_up' from incompatible pointer type
err = security_inode_copy_up(dentry, &old_creds);
^
In file included from fs/overlayfs/copy_up.c:16:0:
include/linux/security.h:762:19: note: expected 'struct dentry *' but argument is of type 'const struct cred **'
static inline int security_inode_copy_up(struct dentry *src, struct dentry *dst)
^
vim +/security_inode_copy_up +262 fs/overlayfs/copy_up.c
246 struct dentry *upper = NULL;
247 umode_t mode = stat->mode;
248 int err;
249 const struct cred *old_creds = NULL;
250
251 newdentry = ovl_lookup_temp(workdir, dentry);
252 err = PTR_ERR(newdentry);
253 if (IS_ERR(newdentry))
254 goto out;
255
256 upper = lookup_one_len(dentry->d_name.name, upperdir,
257 dentry->d_name.len);
258 err = PTR_ERR(upper);
259 if (IS_ERR(upper))
260 goto out1;
261
> 262 err = security_inode_copy_up(dentry, &old_creds);
263 if (err < 0)
264 goto out2;
265
266 /* Can't properly set mode on creation because of the umask */
267 stat->mode &= S_IFMT;
268 err = ovl_create_real(wdir, newdentry, stat, link, NULL, true);
269 stat->mode = mode;
270 if (old_creds)
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 7/5/2016 8:50 AM, Vivek Goyal wrote: > Provide a security hook to label new file correctly when a file is copied > up from lower layer to upper layer of a overlay/union mount. > > This hook can prepare and switch to a new set of creds which are suitable > for new file creation during copy up. Caller should revert to old creds > after file creation. > > In SELinux, newly copied up file gets same label as lower file for > non-context mounts. But it gets label specified in mount option context= > for context mounts. > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > --- > fs/overlayfs/copy_up.c | 8 ++++++++ > include/linux/lsm_hooks.h | 13 +++++++++++++ > include/linux/security.h | 6 ++++++ > security/security.c | 8 ++++++++ > security/selinux/hooks.c | 27 +++++++++++++++++++++++++++ > 5 files changed, 62 insertions(+) > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c > index 80aa6f1..90dc362 100644 > --- a/fs/overlayfs/copy_up.c > +++ b/fs/overlayfs/copy_up.c > @@ -246,6 +246,7 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir, > struct dentry *upper = NULL; > umode_t mode = stat->mode; > int err; > + const struct cred *old_creds = NULL; > > newdentry = ovl_lookup_temp(workdir, dentry); > err = PTR_ERR(newdentry); > @@ -258,10 +259,17 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir, > if (IS_ERR(upper)) > goto out1; > > + err = security_inode_copy_up(dentry, &old_creds); > + if (err < 0) > + goto out2; > + > /* Can't properly set mode on creation because of the umask */ > stat->mode &= S_IFMT; > err = ovl_create_real(wdir, newdentry, stat, link, NULL, true); > stat->mode = mode; > + if (old_creds) > + revert_creds(old_creds); > + > if (err) > goto out2; I don't much care for the way part of the credential manipulation is done in the caller and part is done the the security module. If the caller is going to restore the old state, the caller should save the old state. > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index 7ae3976..fcde9b9 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -401,6 +401,17 @@ > * @inode contains a pointer to the inode. > * @secid contains a pointer to the location where result will be saved. > * In case of failure, @secid will be set to zero. > + * @inode_copy_up: > + * A file is about to be copied up from lower layer to upper layer of > + * overlay filesystem. Prepare a new set of creds and set file creation > + * secid in such a way so that copied up file gets the appropriate The details of what goes on the the SELinux case don't belong here. > + * label. Switch to this newly prepared creds and return old creds. This > + * returns with only one reference to newly prepared creds. So as soon > + * as caller calls revert_cred(old_creds), creds allocated by this hook > + * should be freed. > + * @src indicates the union dentry of file that is being copied up. > + * @old indicates the pointer to old_cred returned to caller. > + * Returns 0 on success or a negative error code on error. > * > * Security hooks for file operations > * > @@ -1425,6 +1436,7 @@ union security_list_options { > int (*inode_listsecurity)(struct inode *inode, char *buffer, > size_t buffer_size); > void (*inode_getsecid)(struct inode *inode, u32 *secid); > + int (*inode_copy_up) (struct dentry *src, const struct cred **old); > > int (*file_permission)(struct file *file, int mask); > int (*file_alloc_security)(struct file *file); > @@ -1696,6 +1708,7 @@ struct security_hook_heads { > struct list_head inode_setsecurity; > struct list_head inode_listsecurity; > struct list_head inode_getsecid; > + struct list_head inode_copy_up; > struct list_head file_permission; > struct list_head file_alloc_security; > struct list_head file_free_security; > diff --git a/include/linux/security.h b/include/linux/security.h > index 14df373..3445df2 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -282,6 +282,7 @@ int security_inode_getsecurity(struct inode *inode, const char *name, void **buf > int security_inode_setsecurity(struct inode *inode, const char *name, const void *value, size_t size, int flags); > int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size); > void security_inode_getsecid(struct inode *inode, u32 *secid); > +int security_inode_copy_up(struct dentry *src, const struct cred **old); > int security_file_permission(struct file *file, int mask); > int security_file_alloc(struct file *file); > void security_file_free(struct file *file); > @@ -758,6 +759,11 @@ static inline void security_inode_getsecid(struct inode *inode, u32 *secid) > *secid = 0; > } > > +static inline int security_inode_copy_up(struct dentry *src, struct dentry *dst) > +{ > + return 0; > +} > + > static inline int security_file_permission(struct file *file, int mask) > { > return 0; > diff --git a/security/security.c b/security/security.c > index 7095693..7c1ce29 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -727,6 +727,12 @@ void security_inode_getsecid(struct inode *inode, u32 *secid) > call_void_hook(inode_getsecid, inode, secid); > } > > +int security_inode_copy_up(struct dentry *src, const struct cred **old) > +{ > + return call_int_hook(inode_copy_up, 0, src, old); > +} > +EXPORT_SYMBOL(security_inode_copy_up); > + > int security_file_permission(struct file *file, int mask) > { > int ret; > @@ -1663,6 +1669,8 @@ struct security_hook_heads security_hook_heads = { > LIST_HEAD_INIT(security_hook_heads.inode_listsecurity), > .inode_getsecid = > LIST_HEAD_INIT(security_hook_heads.inode_getsecid), > + .inode_copy_up = > + LIST_HEAD_INIT(security_hook_heads.inode_copy_up), > .file_permission = > LIST_HEAD_INIT(security_hook_heads.file_permission), > .file_alloc_security = > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index a86d537..1b1a1e5 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -3270,6 +3270,32 @@ static void selinux_inode_getsecid(struct inode *inode, u32 *secid) > *secid = isec->sid; > } > > +static int selinux_inode_copy_up(struct dentry *src, const struct cred **old) > +{ > + u32 sid; > + struct cred *new_creds; > + struct task_security_struct *tsec; > + > + new_creds = prepare_creds(); > + if (!new_creds) > + return -ENOMEM; > + > + /* Get label from overlay inode and set it in create_sid */ > + selinux_inode_getsecid(d_inode(src), &sid); > + tsec = new_creds->security; > + tsec->create_sid = sid; > + *old = override_creds(new_creds); > + > + /* > + * At this point of time we have 2 refs on new_creds. One by > + * prepare_creds and other by override_creds. Drop one reference > + * so that as soon as caller calls revert_creds(old), this cred > + * will be freed. > + */ > + put_cred(new_creds); > + return 0; > +} > + > /* file security operations */ > > static int selinux_revalidate_file_permission(struct file *file, int mask) > @@ -6056,6 +6082,7 @@ static struct security_hook_list selinux_hooks[] = { > LSM_HOOK_INIT(inode_setsecurity, selinux_inode_setsecurity), > LSM_HOOK_INIT(inode_listsecurity, selinux_inode_listsecurity), > LSM_HOOK_INIT(inode_getsecid, selinux_inode_getsecid), > + LSM_HOOK_INIT(inode_copy_up, selinux_inode_copy_up), > > LSM_HOOK_INIT(file_permission, selinux_file_permission), > LSM_HOOK_INIT(file_alloc_security, selinux_file_alloc_security), -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 05, 2016 at 12:36:17PM -0700, Casey Schaufler wrote: > > On 7/5/2016 8:50 AM, Vivek Goyal wrote: > > Provide a security hook to label new file correctly when a file is copied > > up from lower layer to upper layer of a overlay/union mount. > > > > This hook can prepare and switch to a new set of creds which are suitable > > for new file creation during copy up. Caller should revert to old creds > > after file creation. > > > > In SELinux, newly copied up file gets same label as lower file for > > non-context mounts. But it gets label specified in mount option context= > > for context mounts. > > > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > > --- > > fs/overlayfs/copy_up.c | 8 ++++++++ > > include/linux/lsm_hooks.h | 13 +++++++++++++ > > include/linux/security.h | 6 ++++++ > > security/security.c | 8 ++++++++ > > security/selinux/hooks.c | 27 +++++++++++++++++++++++++++ > > 5 files changed, 62 insertions(+) > > > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c > > index 80aa6f1..90dc362 100644 > > --- a/fs/overlayfs/copy_up.c > > +++ b/fs/overlayfs/copy_up.c > > @@ -246,6 +246,7 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir, > > struct dentry *upper = NULL; > > umode_t mode = stat->mode; > > int err; > > + const struct cred *old_creds = NULL; > > > > newdentry = ovl_lookup_temp(workdir, dentry); > > err = PTR_ERR(newdentry); > > @@ -258,10 +259,17 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir, > > if (IS_ERR(upper)) > > goto out1; > > > > + err = security_inode_copy_up(dentry, &old_creds); > > + if (err < 0) > > + goto out2; > > + > > /* Can't properly set mode on creation because of the umask */ > > stat->mode &= S_IFMT; > > err = ovl_create_real(wdir, newdentry, stat, link, NULL, true); > > stat->mode = mode; > > + if (old_creds) > > + revert_creds(old_creds); > > + > > if (err) > > goto out2; > > I don't much care for the way part of the credential manipulation > is done in the caller and part is done the the security module. > If the caller is going to restore the old state, the caller should > save the old state. Ok, I am fine either way. Smalley had preferred switching creds in security module, that's why I did it this way. I will change back to allocating and overriding creds in caller. > > > > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > > index 7ae3976..fcde9b9 100644 > > --- a/include/linux/lsm_hooks.h > > +++ b/include/linux/lsm_hooks.h > > @@ -401,6 +401,17 @@ > > * @inode contains a pointer to the inode. > > * @secid contains a pointer to the location where result will be saved. > > * In case of failure, @secid will be set to zero. > > + * @inode_copy_up: > > + * A file is about to be copied up from lower layer to upper layer of > > + * overlay filesystem. Prepare a new set of creds and set file creation > > + * secid in such a way so that copied up file gets the appropriate > > The details of what goes on the the SELinux case don't belong here. Ok, will remove selinux specific details from here. Thanks Vivek -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 5, 2016 at 11:50 AM, Vivek Goyal <vgoyal@redhat.com> wrote: > Provide a security hook to label new file correctly when a file is copied > up from lower layer to upper layer of a overlay/union mount. > > This hook can prepare and switch to a new set of creds which are suitable > for new file creation during copy up. Caller should revert to old creds > after file creation. > > In SELinux, newly copied up file gets same label as lower file for > non-context mounts. But it gets label specified in mount option context= > for context mounts. > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > --- > fs/overlayfs/copy_up.c | 8 ++++++++ > include/linux/lsm_hooks.h | 13 +++++++++++++ > include/linux/security.h | 6 ++++++ > security/security.c | 8 ++++++++ > security/selinux/hooks.c | 27 +++++++++++++++++++++++++++ > 5 files changed, 62 insertions(+) .. > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index a86d537..1b1a1e5 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -3270,6 +3270,32 @@ static void selinux_inode_getsecid(struct inode *inode, u32 *secid) > *secid = isec->sid; > } > > +static int selinux_inode_copy_up(struct dentry *src, const struct cred **old) > +{ > + u32 sid; > + struct cred *new_creds; > + struct task_security_struct *tsec; > + > + new_creds = prepare_creds(); > + if (!new_creds) > + return -ENOMEM; > + > + /* Get label from overlay inode and set it in create_sid */ > + selinux_inode_getsecid(d_inode(src), &sid); > + tsec = new_creds->security; > + tsec->create_sid = sid; > + *old = override_creds(new_creds); > + > + /* > + * At this point of time we have 2 refs on new_creds. One by > + * prepare_creds and other by override_creds. Drop one reference > + * so that as soon as caller calls revert_creds(old), this cred > + * will be freed. > + */ > + put_cred(new_creds); > + return 0; > +} One quick point of clarification: in addition to the SELinux specific comments in lsm_hooks.h, I think it would be a good idea if the SELinux hook implementation, e.g. security/selinux/hooks.c, was in its own patch and not part of the hook definition. Beyond that, I'm not overly excited about reusing create_sid for this purpose. I understand why you did it, but what if the process had explicitly set create_sid? I think I would prefer the creation of a new field (create_up_sid?) to track this new label and then add an additional check to selinux_inode_init_security() to prefer the existing create_sid to this new field when both are set. Sound reasonable?
On Tue, Jul 05, 2016 at 05:35:22PM -0400, Paul Moore wrote: > On Tue, Jul 5, 2016 at 11:50 AM, Vivek Goyal <vgoyal@redhat.com> wrote: > > Provide a security hook to label new file correctly when a file is copied > > up from lower layer to upper layer of a overlay/union mount. > > > > This hook can prepare and switch to a new set of creds which are suitable > > for new file creation during copy up. Caller should revert to old creds > > after file creation. > > > > In SELinux, newly copied up file gets same label as lower file for > > non-context mounts. But it gets label specified in mount option context= > > for context mounts. > > > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > > --- > > fs/overlayfs/copy_up.c | 8 ++++++++ > > include/linux/lsm_hooks.h | 13 +++++++++++++ > > include/linux/security.h | 6 ++++++ > > security/security.c | 8 ++++++++ > > security/selinux/hooks.c | 27 +++++++++++++++++++++++++++ > > 5 files changed, 62 insertions(+) > > .. > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > index a86d537..1b1a1e5 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -3270,6 +3270,32 @@ static void selinux_inode_getsecid(struct inode *inode, u32 *secid) > > *secid = isec->sid; > > } > > > > +static int selinux_inode_copy_up(struct dentry *src, const struct cred **old) > > +{ > > + u32 sid; > > + struct cred *new_creds; > > + struct task_security_struct *tsec; > > + > > + new_creds = prepare_creds(); > > + if (!new_creds) > > + return -ENOMEM; > > + > > + /* Get label from overlay inode and set it in create_sid */ > > + selinux_inode_getsecid(d_inode(src), &sid); > > + tsec = new_creds->security; > > + tsec->create_sid = sid; > > + *old = override_creds(new_creds); > > + > > + /* > > + * At this point of time we have 2 refs on new_creds. One by > > + * prepare_creds and other by override_creds. Drop one reference > > + * so that as soon as caller calls revert_creds(old), this cred > > + * will be freed. > > + */ > > + put_cred(new_creds); > > + return 0; > > +} > > One quick point of clarification: in addition to the SELinux specific > comments in lsm_hooks.h, I think it would be a good idea if the > SELinux hook implementation, e.g. security/selinux/hooks.c, was in its > own patch and not part of the hook definition. Ok, may be I will break every hook in three parts. - lsm hook declaration. - selinux implementation - overlay implementation of call to hook. > > Beyond that, I'm not overly excited about reusing create_sid for this > purpose. I understand why you did it, but what if the process had > explicitly set create_sid? When a file is copied up, either we retain the label of lower file or set the new label from context=. If any create_sid is set in task, that's ignored. And as we are setting create_sid in a new set of credentials, task will get to retain its create_sid for future operations. As task does not know we are creating a new file, create_sid of task should not matter at all. Task does not know if file is on upper or file is being copied up. For task this file already exists, so task should not expect create_sid label to be present. Am I missing something. Vivek > I think I would prefer the creation of a > new field (create_up_sid?) to track this new label and then add an > additional check to selinux_inode_init_security() to prefer the > existing create_sid to this new field when both are set. > > Sound reasonable? > > -- > paul moore > security @ redhat -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 5, 2016 at 5:52 PM, Vivek Goyal <vgoyal@redhat.com> wrote: > On Tue, Jul 05, 2016 at 05:35:22PM -0400, Paul Moore wrote: >> On Tue, Jul 5, 2016 at 11:50 AM, Vivek Goyal <vgoyal@redhat.com> wrote: >> > Provide a security hook to label new file correctly when a file is copied >> > up from lower layer to upper layer of a overlay/union mount. >> > >> > This hook can prepare and switch to a new set of creds which are suitable >> > for new file creation during copy up. Caller should revert to old creds >> > after file creation. >> > >> > In SELinux, newly copied up file gets same label as lower file for >> > non-context mounts. But it gets label specified in mount option context= >> > for context mounts. >> > >> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> >> > --- >> > fs/overlayfs/copy_up.c | 8 ++++++++ >> > include/linux/lsm_hooks.h | 13 +++++++++++++ >> > include/linux/security.h | 6 ++++++ >> > security/security.c | 8 ++++++++ >> > security/selinux/hooks.c | 27 +++++++++++++++++++++++++++ >> > 5 files changed, 62 insertions(+) >> >> .. >> >> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >> > index a86d537..1b1a1e5 100644 >> > --- a/security/selinux/hooks.c >> > +++ b/security/selinux/hooks.c >> > @@ -3270,6 +3270,32 @@ static void selinux_inode_getsecid(struct inode *inode, u32 *secid) >> > *secid = isec->sid; >> > } >> > >> > +static int selinux_inode_copy_up(struct dentry *src, const struct cred **old) >> > +{ >> > + u32 sid; >> > + struct cred *new_creds; >> > + struct task_security_struct *tsec; >> > + >> > + new_creds = prepare_creds(); >> > + if (!new_creds) >> > + return -ENOMEM; >> > + >> > + /* Get label from overlay inode and set it in create_sid */ >> > + selinux_inode_getsecid(d_inode(src), &sid); >> > + tsec = new_creds->security; >> > + tsec->create_sid = sid; >> > + *old = override_creds(new_creds); >> > + >> > + /* >> > + * At this point of time we have 2 refs on new_creds. One by >> > + * prepare_creds and other by override_creds. Drop one reference >> > + * so that as soon as caller calls revert_creds(old), this cred >> > + * will be freed. >> > + */ >> > + put_cred(new_creds); >> > + return 0; >> > +} ... >> Beyond that, I'm not overly excited about reusing create_sid for this >> purpose. I understand why you did it, but what if the process had >> explicitly set create_sid? > > When a file is copied up, either we retain the label of lower file or > set the new label from context=. If any create_sid is set in task, that's > ignored. > > And as we are setting create_sid in a new set of credentials, task will > get to retain its create_sid for future operations. > > As task does not know we are creating a new file, create_sid of task > should not matter at all. Task does not know if file is on upper or > file is being copied up. For task this file already exists, so task > should not expect create_sid label to be present. > > Am I missing something. I forgot that you are manufacturing a new set of credentials; I must have lost track of that when I was walking through some of the VFS code, my mistake. I'm still rather uneasy about this, but at least you aren't overwriting a previously stored create_sid value.
On Tue, Jul 05, 2016 at 12:36:17PM -0700, Casey Schaufler wrote: > > On 7/5/2016 8:50 AM, Vivek Goyal wrote: > > Provide a security hook to label new file correctly when a file is copied > > up from lower layer to upper layer of a overlay/union mount. > > > > This hook can prepare and switch to a new set of creds which are suitable > > for new file creation during copy up. Caller should revert to old creds > > after file creation. > > > > In SELinux, newly copied up file gets same label as lower file for > > non-context mounts. But it gets label specified in mount option context= > > for context mounts. > > > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > > --- > > fs/overlayfs/copy_up.c | 8 ++++++++ > > include/linux/lsm_hooks.h | 13 +++++++++++++ > > include/linux/security.h | 6 ++++++ > > security/security.c | 8 ++++++++ > > security/selinux/hooks.c | 27 +++++++++++++++++++++++++++ > > 5 files changed, 62 insertions(+) > > > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c > > index 80aa6f1..90dc362 100644 > > --- a/fs/overlayfs/copy_up.c > > +++ b/fs/overlayfs/copy_up.c > > @@ -246,6 +246,7 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir, > > struct dentry *upper = NULL; > > umode_t mode = stat->mode; > > int err; > > + const struct cred *old_creds = NULL; > > > > newdentry = ovl_lookup_temp(workdir, dentry); > > err = PTR_ERR(newdentry); > > @@ -258,10 +259,17 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir, > > if (IS_ERR(upper)) > > goto out1; > > > > + err = security_inode_copy_up(dentry, &old_creds); > > + if (err < 0) > > + goto out2; > > + > > /* Can't properly set mode on creation because of the umask */ > > stat->mode &= S_IFMT; > > err = ovl_create_real(wdir, newdentry, stat, link, NULL, true); > > stat->mode = mode; > > + if (old_creds) > > + revert_creds(old_creds); > > + > > if (err) > > goto out2; > > I don't much care for the way part of the credential manipulation > is done in the caller and part is done the the security module. > If the caller is going to restore the old state, the caller should > save the old state. One advantage of current patches is that we switch to new creds only if it is needed. For example, if there are no LSMs loaded, then there is no need to modify creds and make a switch to new creds. But if I start allocating new creds and save old state in caller, then caller always has to do it (irrespective of the fact whether any LSM modified the creds or not). Thanks Vivek -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7/7/2016 1:33 PM, Vivek Goyal wrote: > On Tue, Jul 05, 2016 at 12:36:17PM -0700, Casey Schaufler wrote: >> On 7/5/2016 8:50 AM, Vivek Goyal wrote: >>> Provide a security hook to label new file correctly when a file is copied >>> up from lower layer to upper layer of a overlay/union mount. >>> >>> This hook can prepare and switch to a new set of creds which are suitable >>> for new file creation during copy up. Caller should revert to old creds >>> after file creation. >>> >>> In SELinux, newly copied up file gets same label as lower file for >>> non-context mounts. But it gets label specified in mount option context= >>> for context mounts. >>> >>> Signed-off-by: Vivek Goyal <vgoyal@redhat.com> >>> --- >>> fs/overlayfs/copy_up.c | 8 ++++++++ >>> include/linux/lsm_hooks.h | 13 +++++++++++++ >>> include/linux/security.h | 6 ++++++ >>> security/security.c | 8 ++++++++ >>> security/selinux/hooks.c | 27 +++++++++++++++++++++++++++ >>> 5 files changed, 62 insertions(+) >>> >>> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c >>> index 80aa6f1..90dc362 100644 >>> --- a/fs/overlayfs/copy_up.c >>> +++ b/fs/overlayfs/copy_up.c >>> @@ -246,6 +246,7 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir, >>> struct dentry *upper = NULL; >>> umode_t mode = stat->mode; >>> int err; >>> + const struct cred *old_creds = NULL; >>> >>> newdentry = ovl_lookup_temp(workdir, dentry); >>> err = PTR_ERR(newdentry); >>> @@ -258,10 +259,17 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir, >>> if (IS_ERR(upper)) >>> goto out1; >>> >>> + err = security_inode_copy_up(dentry, &old_creds); >>> + if (err < 0) >>> + goto out2; >>> + >>> /* Can't properly set mode on creation because of the umask */ >>> stat->mode &= S_IFMT; >>> err = ovl_create_real(wdir, newdentry, stat, link, NULL, true); >>> stat->mode = mode; >>> + if (old_creds) >>> + revert_creds(old_creds); >>> + >>> if (err) >>> goto out2; >> I don't much care for the way part of the credential manipulation >> is done in the caller and part is done the the security module. >> If the caller is going to restore the old state, the caller should >> save the old state. > One advantage of current patches is that we switch to new creds only if > it is needed. For example, if there are no LSMs loaded, Point. > then there is > no need to modify creds and make a switch to new creds. I'm not a fan of cred flipping. There are too many ways for it to go wrong. Consider interrupts. I assume you've ruled that out as a possibility in the caller, but I still think the practice is dangerous. I greatly prefer "create and set attributes" to "change cred, create and reset cred". I know that has it's own set of problems, including races and faking privilege. > But if I start allocating new creds and save old state in caller, then > caller always has to do it (irrespective of the fact whether any LSM > modified the creds or not). It starts getting messy when I have two modules that want to change change the credential. Each module will have to check to see if a module called before it has allocated a new cred. > > Thanks > Vivek > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 7, 2016 at 11:44 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: > On 7/7/2016 1:33 PM, Vivek Goyal wrote: >> On Tue, Jul 05, 2016 at 12:36:17PM -0700, Casey Schaufler wrote: >>> On 7/5/2016 8:50 AM, Vivek Goyal wrote: >>>> Provide a security hook to label new file correctly when a file is copied >>>> up from lower layer to upper layer of a overlay/union mount. >>>> >>>> This hook can prepare and switch to a new set of creds which are suitable >>>> for new file creation during copy up. Caller should revert to old creds >>>> after file creation. >>>> >>>> In SELinux, newly copied up file gets same label as lower file for >>>> non-context mounts. But it gets label specified in mount option context= >>>> for context mounts. >>>> >>>> Signed-off-by: Vivek Goyal <vgoyal@redhat.com> >>>> --- >>>> fs/overlayfs/copy_up.c | 8 ++++++++ >>>> include/linux/lsm_hooks.h | 13 +++++++++++++ >>>> include/linux/security.h | 6 ++++++ >>>> security/security.c | 8 ++++++++ >>>> security/selinux/hooks.c | 27 +++++++++++++++++++++++++++ >>>> 5 files changed, 62 insertions(+) >>>> >>>> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c >>>> index 80aa6f1..90dc362 100644 >>>> --- a/fs/overlayfs/copy_up.c >>>> +++ b/fs/overlayfs/copy_up.c >>>> @@ -246,6 +246,7 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir, >>>> struct dentry *upper = NULL; >>>> umode_t mode = stat->mode; >>>> int err; >>>> + const struct cred *old_creds = NULL; >>>> >>>> newdentry = ovl_lookup_temp(workdir, dentry); >>>> err = PTR_ERR(newdentry); >>>> @@ -258,10 +259,17 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir, >>>> if (IS_ERR(upper)) >>>> goto out1; >>>> >>>> + err = security_inode_copy_up(dentry, &old_creds); >>>> + if (err < 0) >>>> + goto out2; >>>> + >>>> /* Can't properly set mode on creation because of the umask */ >>>> stat->mode &= S_IFMT; >>>> err = ovl_create_real(wdir, newdentry, stat, link, NULL, true); >>>> stat->mode = mode; >>>> + if (old_creds) >>>> + revert_creds(old_creds); >>>> + >>>> if (err) >>>> goto out2; >>> I don't much care for the way part of the credential manipulation >>> is done in the caller and part is done the the security module. >>> If the caller is going to restore the old state, the caller should >>> save the old state. Conversely if the SM is setting the state it should restore it. This needs yet another hook, but that's fine, I think. >> One advantage of current patches is that we switch to new creds only if >> it is needed. For example, if there are no LSMs loaded, > > Point. > >> then there is >> no need to modify creds and make a switch to new creds. > > I'm not a fan of cred flipping. There are too many ways for it to go > wrong. Consider interrupts. I assume you've ruled that out as a possibility > in the caller, but I still think the practice is dangerous. > > I greatly prefer "create and set attributes" to "change cred, create and > reset cred". I know that has it's own set of problems, including races > and faking privilege. Yeah, we've talked about this. The races can be eliminated by always doing the create in a the temporary "workdir" area and atomically renaming to the final destination after everything has been set up. OTOH that has a performance impact that the cred flipping eliminates. >> But if I start allocating new creds and save old state in caller, then >> caller always has to do it (irrespective of the fact whether any LSM >> modified the creds or not). > > It starts getting messy when I have two modules that want to > change change the credential. Each module will have to check to > see if a module called before it has allocated a new cred. Doesn't seem to me too difficult: check if *credp == NULL and allocate if so. Can even invent a heper for this if needed. Thanks, Miklos -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 08, 2016 at 09:21:13AM +0200, Miklos Szeredi wrote: > On Thu, Jul 7, 2016 at 11:44 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: > > On 7/7/2016 1:33 PM, Vivek Goyal wrote: > >> On Tue, Jul 05, 2016 at 12:36:17PM -0700, Casey Schaufler wrote: > >>> On 7/5/2016 8:50 AM, Vivek Goyal wrote: > >>>> Provide a security hook to label new file correctly when a file is copied > >>>> up from lower layer to upper layer of a overlay/union mount. > >>>> > >>>> This hook can prepare and switch to a new set of creds which are suitable > >>>> for new file creation during copy up. Caller should revert to old creds > >>>> after file creation. > >>>> > >>>> In SELinux, newly copied up file gets same label as lower file for > >>>> non-context mounts. But it gets label specified in mount option context= > >>>> for context mounts. > >>>> > >>>> Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > >>>> --- > >>>> fs/overlayfs/copy_up.c | 8 ++++++++ > >>>> include/linux/lsm_hooks.h | 13 +++++++++++++ > >>>> include/linux/security.h | 6 ++++++ > >>>> security/security.c | 8 ++++++++ > >>>> security/selinux/hooks.c | 27 +++++++++++++++++++++++++++ > >>>> 5 files changed, 62 insertions(+) > >>>> > >>>> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c > >>>> index 80aa6f1..90dc362 100644 > >>>> --- a/fs/overlayfs/copy_up.c > >>>> +++ b/fs/overlayfs/copy_up.c > >>>> @@ -246,6 +246,7 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir, > >>>> struct dentry *upper = NULL; > >>>> umode_t mode = stat->mode; > >>>> int err; > >>>> + const struct cred *old_creds = NULL; > >>>> > >>>> newdentry = ovl_lookup_temp(workdir, dentry); > >>>> err = PTR_ERR(newdentry); > >>>> @@ -258,10 +259,17 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir, > >>>> if (IS_ERR(upper)) > >>>> goto out1; > >>>> > >>>> + err = security_inode_copy_up(dentry, &old_creds); > >>>> + if (err < 0) > >>>> + goto out2; > >>>> + > >>>> /* Can't properly set mode on creation because of the umask */ > >>>> stat->mode &= S_IFMT; > >>>> err = ovl_create_real(wdir, newdentry, stat, link, NULL, true); > >>>> stat->mode = mode; > >>>> + if (old_creds) > >>>> + revert_creds(old_creds); > >>>> + > >>>> if (err) > >>>> goto out2; > >>> I don't much care for the way part of the credential manipulation > >>> is done in the caller and part is done the the security module. > >>> If the caller is going to restore the old state, the caller should > >>> save the old state. > > Conversely if the SM is setting the state it should restore it. > This needs yet another hook, but that's fine, I think. > > >> One advantage of current patches is that we switch to new creds only if > >> it is needed. For example, if there are no LSMs loaded, > > > > Point. > > > >> then there is > >> no need to modify creds and make a switch to new creds. > > > > I'm not a fan of cred flipping. There are too many ways for it to go > > wrong. Consider interrupts. I assume you've ruled that out as a possibility > > in the caller, but I still think the practice is dangerous. > > > > I greatly prefer "create and set attributes" to "change cred, create and > > reset cred". I know that has it's own set of problems, including races > > and faking privilege. > > Yeah, we've talked about this. The races can be eliminated by always > doing the create in a the temporary "workdir" area and atomically > renaming to the final destination after everything has been set up. > OTOH that has a performance impact that the cred flipping eliminates. > > >> But if I start allocating new creds and save old state in caller, then > >> caller always has to do it (irrespective of the fact whether any LSM > >> modified the creds or not). > > > > It starts getting messy when I have two modules that want to > > change change the credential. Each module will have to check to > > see if a module called before it has allocated a new cred. > > Doesn't seem to me too difficult: check if *credp == NULL and allocate > if so. Can even invent a heper for this if needed. Right. I like this approach. So cred allocation happens in LSM and switching to new creds and freeing of new creds is done by caller. That way, if no new creds are allocated, then caller does not have to switch creds. Also all LSMs can work on single copy of newly allocated cred and modify it. Also all LSMs can check if creds have already been allocated otherwise allocate new one. Thanks Vivek -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c index 80aa6f1..90dc362 100644 --- a/fs/overlayfs/copy_up.c +++ b/fs/overlayfs/copy_up.c @@ -246,6 +246,7 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir, struct dentry *upper = NULL; umode_t mode = stat->mode; int err; + const struct cred *old_creds = NULL; newdentry = ovl_lookup_temp(workdir, dentry); err = PTR_ERR(newdentry); @@ -258,10 +259,17 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir, if (IS_ERR(upper)) goto out1; + err = security_inode_copy_up(dentry, &old_creds); + if (err < 0) + goto out2; + /* Can't properly set mode on creation because of the umask */ stat->mode &= S_IFMT; err = ovl_create_real(wdir, newdentry, stat, link, NULL, true); stat->mode = mode; + if (old_creds) + revert_creds(old_creds); + if (err) goto out2; diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 7ae3976..fcde9b9 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -401,6 +401,17 @@ * @inode contains a pointer to the inode. * @secid contains a pointer to the location where result will be saved. * In case of failure, @secid will be set to zero. + * @inode_copy_up: + * A file is about to be copied up from lower layer to upper layer of + * overlay filesystem. Prepare a new set of creds and set file creation + * secid in such a way so that copied up file gets the appropriate + * label. Switch to this newly prepared creds and return old creds. This + * returns with only one reference to newly prepared creds. So as soon + * as caller calls revert_cred(old_creds), creds allocated by this hook + * should be freed. + * @src indicates the union dentry of file that is being copied up. + * @old indicates the pointer to old_cred returned to caller. + * Returns 0 on success or a negative error code on error. * * Security hooks for file operations * @@ -1425,6 +1436,7 @@ union security_list_options { int (*inode_listsecurity)(struct inode *inode, char *buffer, size_t buffer_size); void (*inode_getsecid)(struct inode *inode, u32 *secid); + int (*inode_copy_up) (struct dentry *src, const struct cred **old); int (*file_permission)(struct file *file, int mask); int (*file_alloc_security)(struct file *file); @@ -1696,6 +1708,7 @@ struct security_hook_heads { struct list_head inode_setsecurity; struct list_head inode_listsecurity; struct list_head inode_getsecid; + struct list_head inode_copy_up; struct list_head file_permission; struct list_head file_alloc_security; struct list_head file_free_security; diff --git a/include/linux/security.h b/include/linux/security.h index 14df373..3445df2 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -282,6 +282,7 @@ int security_inode_getsecurity(struct inode *inode, const char *name, void **buf int security_inode_setsecurity(struct inode *inode, const char *name, const void *value, size_t size, int flags); int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size); void security_inode_getsecid(struct inode *inode, u32 *secid); +int security_inode_copy_up(struct dentry *src, const struct cred **old); int security_file_permission(struct file *file, int mask); int security_file_alloc(struct file *file); void security_file_free(struct file *file); @@ -758,6 +759,11 @@ static inline void security_inode_getsecid(struct inode *inode, u32 *secid) *secid = 0; } +static inline int security_inode_copy_up(struct dentry *src, struct dentry *dst) +{ + return 0; +} + static inline int security_file_permission(struct file *file, int mask) { return 0; diff --git a/security/security.c b/security/security.c index 7095693..7c1ce29 100644 --- a/security/security.c +++ b/security/security.c @@ -727,6 +727,12 @@ void security_inode_getsecid(struct inode *inode, u32 *secid) call_void_hook(inode_getsecid, inode, secid); } +int security_inode_copy_up(struct dentry *src, const struct cred **old) +{ + return call_int_hook(inode_copy_up, 0, src, old); +} +EXPORT_SYMBOL(security_inode_copy_up); + int security_file_permission(struct file *file, int mask) { int ret; @@ -1663,6 +1669,8 @@ struct security_hook_heads security_hook_heads = { LIST_HEAD_INIT(security_hook_heads.inode_listsecurity), .inode_getsecid = LIST_HEAD_INIT(security_hook_heads.inode_getsecid), + .inode_copy_up = + LIST_HEAD_INIT(security_hook_heads.inode_copy_up), .file_permission = LIST_HEAD_INIT(security_hook_heads.file_permission), .file_alloc_security = diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index a86d537..1b1a1e5 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -3270,6 +3270,32 @@ static void selinux_inode_getsecid(struct inode *inode, u32 *secid) *secid = isec->sid; } +static int selinux_inode_copy_up(struct dentry *src, const struct cred **old) +{ + u32 sid; + struct cred *new_creds; + struct task_security_struct *tsec; + + new_creds = prepare_creds(); + if (!new_creds) + return -ENOMEM; + + /* Get label from overlay inode and set it in create_sid */ + selinux_inode_getsecid(d_inode(src), &sid); + tsec = new_creds->security; + tsec->create_sid = sid; + *old = override_creds(new_creds); + + /* + * At this point of time we have 2 refs on new_creds. One by + * prepare_creds and other by override_creds. Drop one reference + * so that as soon as caller calls revert_creds(old), this cred + * will be freed. + */ + put_cred(new_creds); + return 0; +} + /* file security operations */ static int selinux_revalidate_file_permission(struct file *file, int mask) @@ -6056,6 +6082,7 @@ static struct security_hook_list selinux_hooks[] = { LSM_HOOK_INIT(inode_setsecurity, selinux_inode_setsecurity), LSM_HOOK_INIT(inode_listsecurity, selinux_inode_listsecurity), LSM_HOOK_INIT(inode_getsecid, selinux_inode_getsecid), + LSM_HOOK_INIT(inode_copy_up, selinux_inode_copy_up), LSM_HOOK_INIT(file_permission, selinux_file_permission), LSM_HOOK_INIT(file_alloc_security, selinux_file_alloc_security),
Provide a security hook to label new file correctly when a file is copied up from lower layer to upper layer of a overlay/union mount. This hook can prepare and switch to a new set of creds which are suitable for new file creation during copy up. Caller should revert to old creds after file creation. In SELinux, newly copied up file gets same label as lower file for non-context mounts. But it gets label specified in mount option context= for context mounts. Signed-off-by: Vivek Goyal <vgoyal@redhat.com> --- fs/overlayfs/copy_up.c | 8 ++++++++ include/linux/lsm_hooks.h | 13 +++++++++++++ include/linux/security.h | 6 ++++++ security/security.c | 8 ++++++++ security/selinux/hooks.c | 27 +++++++++++++++++++++++++++ 5 files changed, 62 insertions(+)