On Tue, Jul 05, 2016 at 01:22:22PM -0700, Casey Schaufler wrote:
> On 7/5/2016 8:50 AM, Vivek Goyal wrote:
> > Provide a security hook which is called when xattrs of a file are being
> > copied up. This hook is called once for each xattr and one can either
> > accept or reject xattr. If 0 is returned, xattr will be copied up, if 1
> > is returned, xattr will not be copied up and if negative error code
> > is returned, copy up will be aborted.
> >
> > In SELinux, label of lower file is not copied up. File already has been
> > set with right label at the time of creation and we don't want to overwrite
> > that label.
> >
> > Signed-off-by: David Howells <dhowells@xxxxxxxxxx>
> > Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx>
> > ---
> > fs/overlayfs/copy_up.c | 8 ++++++++
> > include/linux/lsm_hooks.h | 13 +++++++++++++
> > include/linux/security.h | 10 ++++++++++
> > security/security.c | 9 +++++++++
> > security/selinux/hooks.c | 14 ++++++++++++++
> > 5 files changed, 54 insertions(+)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index 90dc362..2c31938 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -103,6 +103,14 @@ retry:
> > goto retry;
> > }
> >
> > + error = security_inode_copy_up_xattr(old, new,
> > + name, value, size);
> > + if (error < 0)
> > + break;
> > + if (error == 1) {
> > + error = 0;
> > + continue; /* Discard */
> > + }
> > error = vfs_setxattr(new, name, value, size, 0);
> > if (error)
> > break;
> > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > index fcde9b9..2a8ee8c 100644
> > --- a/include/linux/lsm_hooks.h
> > +++ b/include/linux/lsm_hooks.h
> > @@ -412,6 +412,16 @@
> > * @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.
> > + * @inode_copy_up_xattr:
> > + * Filter the xattrs being copied up when a unioned file is copied
> > + * up from a lower layer to the union/overlay layer.
> > + * @src indicates the file that is being copied up.
> > + * @dst indicates the file that has being created by the copy up.
> > + * @name indicates the name of the xattr.
> > + * @value, @size indicate the payload of the xattr.
> > + * Returns 0 to accept the xattr, 1 to discard the xattr or a negative
> > + * error code to abort the copy up. Note that the caller is responsible
> > + * for reading and writing the xattrs as this hook is merely a filter.
>
> The return should be -EOPNOTSUPP from security modules that don't
> support the attribute "name". This will make it possible to support
> multiple modules that provide attributes. (patches pending)
Hmm.., Sorry I did not understand this one.
So all modules will not understand all xattrs. So if they start returning
-EOPNOTSUPP, then as per current implementation, copy up operation will
be aborted.
Current implementation relies on that a security module, returns 0 if
every thing is "name" xattr should be copied up or lsm does not care.
Negative error code is returned only if something is wrong. Given every
lsm will not understand/care about all the xattrs, we can't return
error code if lsm does not own/understand the "name". In fact
call_int_hook() will bail out the very first time negative error code
is returned.
IOW, current implementation will work with multiple modules providing
implementation for same hook as long as module returns 0 for the xattrs
it does not understand.
I guess I am missing something. Can you please elaborate a little more.
>
> If the only use to which this hook is put is to identify attributes
> that should be discarded it's unnecessary overhead to pass the
> parameters that are never used.
Ok, I will get rid of extra parameters. If somebody needs these, it can
be added later.
Vivek
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
