[Cluster-devel] [PATCH v5] Add flags option to get xattr method paired to __vfs_getxattr
Replace arguments for get and set xattr methods, and __vfs_getxattr
and __vfs_setaxtr functions with a reference to the following
argument structure:
struct xattr_gs_args {
struct dentry *dentry;
struct inode *inode;
const char *name;
union {
void *buffer;
const void *value;
};
size_t size;
int flags;
};
Which in effect adds a flags option to the get method and
__vfs_getxattr function.
Add a flag option to get xattr method that has bit flag of
XATTR_NOSECURITY passed to it. XATTR_NOSECURITY is generally then
set in the __vfs_getxattr path when called by security
infrastructure.
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_NOSECURITY}) ->
handler->get({dentry...XATTR_NOSECURITY}) ->
__vfs_getxattr({lower_dentry...XATTR_NOSECURITY}) ->
lower_handler->get({lower_dentry...XATTR_NOSECURITY})
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: sta...@vger.kernel.org # 4.4, 4.9, 4.14 & 4.19
---
v5:
- introduce struct xattr_gs_args for get and set methods,
__vfs_getxattr and __vfs_setxattr functions.
- cover a missing spot in ext2.
- switch from snprintf to scnprintf for correctness.
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.
---
Documentation/filesystems/Locking | 10 ++-
drivers/staging/erofs/xattr.c | 8 +--
fs/9p/acl.c | 37 +-
fs/9p/xattr.c | 19 +++--
fs/afs/xattr.c| 110 +
fs/btrfs/xattr.c | 36 +-
fs/ceph/xattr.c | 40 +--
fs/cifs/xattr.c | 72 +--
fs/ecryptfs/crypto.c | 20 +++---
fs/ecryptfs/inode.c | 36 ++
fs/ecryptfs/mmap.c| 39 ++-
fs/ext2/xattr_security.c | 16 ++---
fs/ext2/xattr_trusted.c | 15 ++--
fs/ext2/xattr_user.c | 19 +++--
fs/ext4/xattr_security.c | 15 ++--
fs/ext4/xattr_trusted.c | 15 ++--
fs/ext4/xattr_user.c | 19 +++--
fs/f2fs/xattr.c | 42 +--
fs/fuse/xattr.c | 23 +++---
fs/gfs2/xattr.c | 18 ++---
fs/hfs/attr.c | 15 ++--
fs/hfsplus/xattr.c| 17 +++--
fs/hfsplus/xattr_security.c | 13 ++--
fs/hfsplus/xattr_trusted.c| 13 ++--
fs/hfsplus/xattr_user.c | 13 ++--
fs/jffs2/security.c | 16 ++---
fs/jffs2/xattr_trusted.c | 16 ++---
fs/jffs2/xattr_user.c | 16 ++---
fs/jfs/xattr.c| 33 -
fs/kernfs/inode.c | 21 +++---
fs/nfs/nfs4proc.c | 28
fs/ocfs2/xattr.c | 52 ++
fs/orangefs/xattr.c | 19 ++---
fs/overlayfs/inode.c | 43 ++--
fs/overlayfs/overlayfs.h | 6 +-
fs/overlayfs/super.c | 53 ++
fs/posix_acl.c| 23 +++---
fs/reiserfs/xattr.c | 2 +-
fs/reiserfs/xattr_security.c | 22 +++---
fs/reiserfs/xattr_trusted.c | 22 +++---
fs/reiserfs/xattr_user.c | 22 +++---
fs/squashfs/xattr.c | 10 +--
Re: [PATCH v5 03/18] gfs2: add compat_ioctl support
On Mon, Aug 19, 2019 at 11:09 AM Arnd Bergmann wrote:
> On Sun, Aug 18, 2019 at 10:17 PM Andreas Grünbacher
> wrote:
> > Am So., 18. Aug. 2019 um 21:32 Uhr schrieb Arnd Bergmann :
> > > On Fri, Aug 16, 2019 at 7:32 PM Andreas Gruenbacher
> > > wrote:
> > > > On Wed, Aug 14, 2019 at 10:45 PM Arnd Bergmann wrote:
> > > > > + /* 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;
> > >
> > > I've looked at it again: if we do this, the function actually becomes
> > > longer than the native gfs2_ioctl(). Should we just make a full copy then?
> >
> > I don't think the length of gfs2_compat_ioctl is really an issue as
> > long as the function is that simple.
>
> True. The most important goal should just be to make it easy to
> add the correct handler the next time another command is added
> to the ioctl function.
>
> Just let me know which version you want for that:
>
> 1. my original patch
> 2. the version from your reply
That one, please.
> 3. my version below with compat_ptr() added
> 4. ...
>
> > > static long gfs2_compat_ioctl(struct file *filp, unsigned int cmd,
> > > unsigned long arg)
> > > {
> > > switch(cmd) {
> > > case FS_IOC32_GETFLAGS:
> > > return gfs2_get_flags(filp, (u32 __user *)arg);
> > > case FS_IOC32_SETFLAGS:
> > > return gfs2_set_flags(filp, (u32 __user *)arg);
> > > case FITRIM:
> > > return gfs2_fitrim(filp, (void __user *)arg);
> > > case FS_IOC_GETFSLABEL:
> > > return gfs2_getlabel(filp, (char __user *)arg);
> > > }
> > >
> > > return -ENOTTY;
> > > }
> >
> > Don't we still need the compat_ptr conversion? That seems to be the
> > main point of having a compat_ioctl operation.
>
> Right, of course. Fixed now in my tree.
>
> Arnd
Thanks,
Andreas
Re: [PATCH v5 03/18] gfs2: add compat_ioctl support
On Sun, Aug 18, 2019 at 10:17 PM Andreas Grünbacher
wrote:
> Am So., 18. Aug. 2019 um 21:32 Uhr schrieb Arnd Bergmann :
> > On Fri, Aug 16, 2019 at 7:32 PM Andreas Gruenbacher
> > wrote:
> > > On Wed, Aug 14, 2019 at 10:45 PM Arnd Bergmann wrote:
> > > > + /* 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;
> >
> > I've looked at it again: if we do this, the function actually becomes
> > longer than the native gfs2_ioctl(). Should we just make a full copy then?
>
> I don't think the length of gfs2_compat_ioctl is really an issue as
> long as the function is that simple.
True. The most important goal should just be to make it easy to
add the correct handler the next time another command is added
to the ioctl function.
Just let me know which version you want for that:
1. my original patch
2. the version from your reply
3. my version below with compat_ptr() added
4. ...
> > static long gfs2_compat_ioctl(struct file *filp, unsigned int cmd,
> > unsigned long arg)
> > {
> > switch(cmd) {
> > case FS_IOC32_GETFLAGS:
> > return gfs2_get_flags(filp, (u32 __user *)arg);
> > case FS_IOC32_SETFLAGS:
> > return gfs2_set_flags(filp, (u32 __user *)arg);
> > case FITRIM:
> > return gfs2_fitrim(filp, (void __user *)arg);
> > case FS_IOC_GETFSLABEL:
> > return gfs2_getlabel(filp, (char __user *)arg);
> > }
> >
> > return -ENOTTY;
> > }
>
> Don't we still need the compat_ptr conversion? That seems to be the
> main point of having a compat_ioctl operation.
Right, of course. Fixed now in my tree.
Arnd
