Re: [Cluster-devel] [PATCH v5 03/18] gfs2: add compat_ioctl support
Arnd,
On Wed, Aug 14, 2019 at 10:45 PM Arnd Bergmann wrote:
>
> Out of the four ioctl commands supported on gfs2, only FITRIM
> works in compat mode.
>
> Add a proper handler based on the ext4 implementation.
>
> Fixes: 6ddc5c3ddf25 ("gfs2: getlabel support")
> Signed-off-by: Arnd Bergmann
> ---
> fs/gfs2/file.c | 24
> 1 file changed, 24 insertions(+)
>
> diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
> index 52fa1ef8400b..49287f0b96d0 100644
> --- a/fs/gfs2/file.c
> +++ b/fs/gfs2/file.c
> @@ -6,6 +6,7 @@
>
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -354,6 +355,25 @@ static long gfs2_ioctl(struct file *filp, unsigned int
> cmd, unsigned long arg)
> return -ENOTTY;
> }
>
> +#ifdef CONFIG_COMPAT
> +static long gfs2_compat_ioctl(struct file *filp, unsigned int cmd, unsigned
> long arg)
> +{
> + /* These are just misnamed, they actually get/put from/to user an int
> */
> + switch(cmd) {
> + case FS_IOC32_GETFLAGS:
> + cmd = FS_IOC_GETFLAGS;
> + break;
> + case FS_IOC32_SETFLAGS:
> + cmd = FS_IOC_SETFLAGS;
> + break;
I'd like the code to be more explicit here:
case FITRIM:
case FS_IOC_GETFSLABEL:
break;
default:
return -ENOIOCTLCMD;
> + }
> +
> + return gfs2_ioctl(filp, cmd, (unsigned long)compat_ptr(arg));
> +}
> +#else
> +#define gfs2_compat_ioctl NULL
> +#endif
> +
> /**
> * gfs2_size_hint - Give a hint to the size of a write request
> * @filep: The struct file
> @@ -1294,6 +1314,7 @@ const struct file_operations gfs2_file_fops = {
> .write_iter = gfs2_file_write_iter,
> .iopoll = iomap_dio_iopoll,
> .unlocked_ioctl = gfs2_ioctl,
> + .compat_ioctl = gfs2_compat_ioctl,
> .mmap = gfs2_mmap,
> .open = gfs2_open,
> .release= gfs2_release,
> @@ -1309,6 +1330,7 @@ const struct file_operations gfs2_file_fops = {
> const struct file_operations gfs2_dir_fops = {
> .iterate_shared = gfs2_readdir,
> .unlocked_ioctl = gfs2_ioctl,
> + .compat_ioctl = gfs2_compat_ioctl,
> .open = gfs2_open,
> .release= gfs2_release,
> .fsync = gfs2_fsync,
> @@ -1325,6 +1347,7 @@ const struct file_operations gfs2_file_fops_nolock = {
> .write_iter = gfs2_file_write_iter,
> .iopoll = iomap_dio_iopoll,
> .unlocked_ioctl = gfs2_ioctl,
> + .compat_ioctl = gfs2_compat_ioctl,
> .mmap = gfs2_mmap,
> .open = gfs2_open,
> .release= gfs2_release,
> @@ -1338,6 +1361,7 @@ const struct file_operations gfs2_file_fops_nolock = {
> const struct file_operations gfs2_dir_fops_nolock = {
> .iterate_shared = gfs2_readdir,
> .unlocked_ioctl = gfs2_ioctl,
> + .compat_ioctl = gfs2_compat_ioctl,
> .open = gfs2_open,
> .release= gfs2_release,
> .fsync = gfs2_fsync,
> --
> 2.20.0
>
Should we feed this through the gfs2 tree?
Thanks,
Andreas
Re: [Cluster-devel] [PATCH v4] Add flags option to get xattr method paired to __vfs_getxattr
I will redo the manual audit, perhaps I need to not be so scared of allconfig build ;-} (currently my runtime & buildtime test platform is android 4.19 kernel, my bad). I am going through and exploring/changing/testing everything to deal with GregKH/jmorris wanting a single xattr_gs_args structure reference where the flags argument is stowed, rather than YA adding a flags argument. It is doubling the (mechanical) codebase impact but decreases the future maintenance and _large_ set of stakeholders (+75 at last count). -- Mark On Fri, Aug 16, 2019 at 8:11 AM Jan Kara wrote: > On Thu 15-08-19 08:49:58, Mark Salyzyn wrote: > > Add a flag option to get xattr method that could have a bit flag of > > XATTR_NOSECURITY passed to it. XATTR_NOSECURITY is generally then > > set in the __vfs_getxattr path. > > > > This handles the case of a union filesystem driver that is being > > requested by the security layer to report back the xattr data. > > > > For the use case where access is to be blocked by the security layer. > > > > The path then could be security(dentry) -> > > __vfs_getxattr(dentry...XATTR_NOSECUIRTY) -> > > handler->get(dentry...XATTR_NOSECURITY) -> > > __vfs_getxattr(lower_dentry...XATTR_NOSECUIRTY) -> > > lower_handler->get(lower_dentry...XATTR_NOSECUIRTY) > > which would report back through the chain data and success as > > expected, the logging security layer at the top would have the > > data to determine the access permissions and report back the target > > context that was blocked. > > > > Without the get handler flag, the path on a union filesystem would be > > the errant security(dentry) -> __vfs_getxattr(dentry) -> > > handler->get(dentry) -> vfs_getxattr(lower_dentry) -> nested -> > > security(lower_dentry, log off) -> lower_handler->get(lower_dentry) > > which would report back through the chain no data, and -EACCES. > > > > For selinux for both cases, this would translate to a correctly > > determined blocked access. In the first case with this change a correct > avc > > log would be reported, in the second legacy case an incorrect avc log > > would be reported against an uninitialized u:object_r:unlabeled:s0 > > context making the logs cosmetically useless for audit2allow. > > > > This patch series is inert and is the wide-spread addition of the > > flags option for xattr functions, and a replacement of _vfs_getxattr > > with __vfs_getxattr(...XATTR_NOSECURITY). > > > > Signed-off-by: Mark Salyzyn > > Cc: Stephen Smalley > > Cc: linux-ker...@vger.kernel.org > > Cc: kernel-t...@android.com > > Cc: linux-security-mod...@vger.kernel.org > > Cc: Jan Kara > > Cc: sta...@vger.kernel.org # 4.4, 4.9, 4.14 & 4.19 > > The patch looks good to me. I can see you've missed conversion of one place > in ext2 which 0-day spotted so that will need fixing. Otherwise feel free > to add: > > Acked-by: Jan Kara > > Honza > > --- > > v4: > > - ifdef __KERNEL__ around XATTR_NOSECURITY to > > keep it colocated in uapi headers. > > > > v3: > > - poor aim on ubifs not ubifs_xattr_get, but static xattr_get > > > > v2: > > - Missed a spot: ubifs, erofs and afs. > > > > v1: > > - Removed from an overlayfs patch set, and made independent. > > Expect this to be the basis of some security improvements. > > --- > > drivers/staging/erofs/xattr.c | 3 ++- > > fs/9p/acl.c | 3 ++- > > fs/9p/xattr.c | 3 ++- > > fs/afs/xattr.c| 8 +++ > > fs/btrfs/xattr.c | 3 ++- > > fs/ceph/xattr.c | 3 ++- > > fs/cifs/xattr.c | 2 +- > > fs/ecryptfs/inode.c | 6 -- > > fs/ecryptfs/mmap.c| 2 +- > > fs/ext2/xattr_trusted.c | 2 +- > > fs/ext2/xattr_user.c | 2 +- > > fs/ext4/xattr_security.c | 2 +- > > fs/ext4/xattr_trusted.c | 2 +- > > fs/ext4/xattr_user.c | 2 +- > > fs/f2fs/xattr.c | 4 ++-- > > fs/fuse/xattr.c | 4 ++-- > > fs/gfs2/xattr.c | 3 ++- > > fs/hfs/attr.c | 2 +- > > fs/hfsplus/xattr.c| 3 ++- > > fs/hfsplus/xattr_trusted.c| 3 ++- > > fs/hfsplus/xattr_user.c | 3 ++- > > fs/jffs2/security.c | 3 ++- > > fs/jffs2/xattr_trusted.c | 3 ++- > > fs/jffs2/xattr_user.c | 3 ++- > > fs/jfs/xattr.c| 5 +++-- > > fs/kernfs/inode.c | 3 ++- > > fs/nfs/nfs4proc.c | 6 -- > > fs/ocfs2/xattr.c | 9 +--- > > fs/orangefs/xattr.c | 3 ++- > > fs/overlayfs/super.c | 8 --- > > fs/posix_acl.c| 2 +- > > fs/reiserfs/xattr_security.c | 3 ++- > > fs/reiserfs/xattr_trusted.c | 3 ++- > > fs/reiserfs/xattr_user.c | 3 ++- > >
Re: [Cluster-devel] [PATCH] Add flags option to get xattr method paired to __vfs_getxattr
On 8/15/19 3:27 PM, James Morris wrote:
On Thu, 15 Aug 2019, Mark Salyzyn wrote:
Good Idea, but using the same argument structure for set and get I would be
concerned about the loss of compiler protection for the buffer argument;
Agreed, I missed that.
Sadly, the pattern of
struct getxattr_args args;
memset(, 0, sizeof(args));
args. = ...
__vfs_getxattr(};
...
__vfs_setxattr();
would be nice, so maybe we need to cool our jets and instead:
struct xattr_gs_args {
struct dentry *dentry;
struct inode *inode;
const char *name;
union {
void *buffer;
const void *value;
};
size_t size;
int flags;
};
value _must_ be referenced for all setxattr operations, buffer for getxattr
operations (how can we enforce that?).
struct getxattr_args {
struct dentry *dentry;
struct inode *inode;
const char *name;
void *buffer;
size_t size;
int flags;
Does 'get' need flags?
:-) That was the _whole_ point of the patch, flags is how we pass in the
recursion so that a security/internal getxattr call has the rights to
acquire the data in the lower layer(s).
-- Mark
Re: [Cluster-devel] [PATCH v4] Add flags option to get xattr method paired to __vfs_getxattr
On Thu 15-08-19 08:49:58, Mark Salyzyn wrote: > Add a flag option to get xattr method that could have a bit flag of > XATTR_NOSECURITY passed to it. XATTR_NOSECURITY is generally then > set in the __vfs_getxattr path. > > This handles the case of a union filesystem driver that is being > requested by the security layer to report back the xattr data. > > For the use case where access is to be blocked by the security layer. > > The path then could be security(dentry) -> > __vfs_getxattr(dentry...XATTR_NOSECUIRTY) -> > handler->get(dentry...XATTR_NOSECURITY) -> > __vfs_getxattr(lower_dentry...XATTR_NOSECUIRTY) -> > lower_handler->get(lower_dentry...XATTR_NOSECUIRTY) > which would report back through the chain data and success as > expected, the logging security layer at the top would have the > data to determine the access permissions and report back the target > context that was blocked. > > Without the get handler flag, the path on a union filesystem would be > the errant security(dentry) -> __vfs_getxattr(dentry) -> > handler->get(dentry) -> vfs_getxattr(lower_dentry) -> nested -> > security(lower_dentry, log off) -> lower_handler->get(lower_dentry) > which would report back through the chain no data, and -EACCES. > > For selinux for both cases, this would translate to a correctly > determined blocked access. In the first case with this change a correct avc > log would be reported, in the second legacy case an incorrect avc log > would be reported against an uninitialized u:object_r:unlabeled:s0 > context making the logs cosmetically useless for audit2allow. > > This patch series is inert and is the wide-spread addition of the > flags option for xattr functions, and a replacement of _vfs_getxattr > with __vfs_getxattr(...XATTR_NOSECURITY). > > Signed-off-by: Mark Salyzyn > Cc: Stephen Smalley > Cc: linux-ker...@vger.kernel.org > Cc: kernel-t...@android.com > Cc: linux-security-mod...@vger.kernel.org > Cc: Jan Kara > Cc: sta...@vger.kernel.org # 4.4, 4.9, 4.14 & 4.19 The patch looks good to me. I can see you've missed conversion of one place in ext2 which 0-day spotted so that will need fixing. Otherwise feel free to add: Acked-by: Jan Kara Honza > --- > v4: > - ifdef __KERNEL__ around XATTR_NOSECURITY to > keep it colocated in uapi headers. > > v3: > - poor aim on ubifs not ubifs_xattr_get, but static xattr_get > > v2: > - Missed a spot: ubifs, erofs and afs. > > v1: > - Removed from an overlayfs patch set, and made independent. > Expect this to be the basis of some security improvements. > --- > drivers/staging/erofs/xattr.c | 3 ++- > fs/9p/acl.c | 3 ++- > fs/9p/xattr.c | 3 ++- > fs/afs/xattr.c| 8 +++ > fs/btrfs/xattr.c | 3 ++- > fs/ceph/xattr.c | 3 ++- > fs/cifs/xattr.c | 2 +- > fs/ecryptfs/inode.c | 6 -- > fs/ecryptfs/mmap.c| 2 +- > fs/ext2/xattr_trusted.c | 2 +- > fs/ext2/xattr_user.c | 2 +- > fs/ext4/xattr_security.c | 2 +- > fs/ext4/xattr_trusted.c | 2 +- > fs/ext4/xattr_user.c | 2 +- > fs/f2fs/xattr.c | 4 ++-- > fs/fuse/xattr.c | 4 ++-- > fs/gfs2/xattr.c | 3 ++- > fs/hfs/attr.c | 2 +- > fs/hfsplus/xattr.c| 3 ++- > fs/hfsplus/xattr_trusted.c| 3 ++- > fs/hfsplus/xattr_user.c | 3 ++- > fs/jffs2/security.c | 3 ++- > fs/jffs2/xattr_trusted.c | 3 ++- > fs/jffs2/xattr_user.c | 3 ++- > fs/jfs/xattr.c| 5 +++-- > fs/kernfs/inode.c | 3 ++- > fs/nfs/nfs4proc.c | 6 -- > fs/ocfs2/xattr.c | 9 +--- > fs/orangefs/xattr.c | 3 ++- > fs/overlayfs/super.c | 8 --- > fs/posix_acl.c| 2 +- > fs/reiserfs/xattr_security.c | 3 ++- > fs/reiserfs/xattr_trusted.c | 3 ++- > fs/reiserfs/xattr_user.c | 3 ++- > fs/squashfs/xattr.c | 2 +- > fs/ubifs/xattr.c | 3 ++- > fs/xattr.c| 36 +++ > fs/xfs/xfs_xattr.c| 3 ++- > include/linux/xattr.h | 9 > include/uapi/linux/xattr.h| 7 -- > mm/shmem.c| 3 ++- > net/socket.c | 3 ++- > security/commoncap.c | 6 -- > security/integrity/evm/evm_main.c | 3 ++- > security/selinux/hooks.c | 11 ++ > security/smack/smack_lsm.c| 5 +++-- > 46 files changed, 126 insertions(+), 84 deletions(-) > > diff --git a/drivers/staging/erofs/xattr.c b/drivers/staging/erofs/xattr.c > index df40654b9fbb..69440065432c 100644 > ---
Re: [Cluster-devel] [PATCH v4] Add flags option to get xattr method paired to __vfs_getxattr
Hi Mark,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[cannot apply to v5.3-rc4 next-20190814]
[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/Mark-Salyzyn/Add-flags-option-to-get-xattr-method-paired-to-__vfs_getxattr/20190816-155049
config: x86_64-lkp (attached as .config)
compiler: gcc-7 (Debian 7.4.0-10) 7.4.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot
All errors (new ones prefixed by >>):
>> fs/ext2/xattr_security.c:56:9: error: initialization from incompatible
>> pointer type [-Werror=incompatible-pointer-types]
.get = ext2_xattr_security_get,
^~~
fs/ext2/xattr_security.c:56:9: note: (near initialization for
'ext2_xattr_security_handler.get')
cc1: some warnings being treated as errors
vim +56 fs/ext2/xattr_security.c
9d8f13ba3f4833 Mimi Zohar2011-06-06 53
749c72efa4bd91 Stephen Hemminger 2010-05-13 54 const struct xattr_handler
ext2_xattr_security_handler = {
^1da177e4c3f41 Linus Torvalds2005-04-16 55 .prefix =
XATTR_SECURITY_PREFIX,
^1da177e4c3f41 Linus Torvalds2005-04-16 @56 .get=
ext2_xattr_security_get,
:: The code at line 56 was first introduced by commit
:: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 Linux-2.6.12-rc2
:: TO: Linus Torvalds
:: CC: Linus Torvalds
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
