From patchwork Tue Jul 5 15:50:54 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Vivek Goyal X-Patchwork-Id: 9214683 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 7B7F06048B for ; Tue, 5 Jul 2016 15:55:07 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 6CFB02684F for ; Tue, 5 Jul 2016 15:55:07 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 61C5626861; Tue, 5 Jul 2016 15:55:07 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id F2DD726E64 for ; Tue, 5 Jul 2016 15:55:06 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933182AbcGEPyh (ORCPT ); Tue, 5 Jul 2016 11:54:37 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42297 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754856AbcGEPvs (ORCPT ); Tue, 5 Jul 2016 11:51:48 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1973C8535A; Tue, 5 Jul 2016 15:51:48 +0000 (UTC) Received: from horse.redhat.com (dhcp-25-107.bos.redhat.com [10.18.25.107]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u65Fplcn000531; Tue, 5 Jul 2016 11:51:47 -0400 Received: by horse.redhat.com (Postfix, from userid 10451) id BC944205FCC; Tue, 5 Jul 2016 11:51:46 -0400 (EDT) From: Vivek Goyal To: miklos@szeredi.hu, sds@tycho.nsa.gov, linux-kernel@vger.kernel.org, linux-unionfs@vger.kernel.org, linux-security-module@vger.kernel.org Cc: dwalsh@redhat.com, dhowells@redhat.com, pmoore@redhat.com, viro@ZenIV.linux.org.uk, vgoyal@redhat.com, linux-fsdevel@vger.kernel.org Subject: [PATCH 5/5] overlayfs: Use vfs_getxattr_noperm() for real inode Date: Tue, 5 Jul 2016 11:50:54 -0400 Message-Id: <1467733854-6314-6-git-send-email-vgoyal@redhat.com> In-Reply-To: <1467733854-6314-1-git-send-email-vgoyal@redhat.com> References: <1467733854-6314-1-git-send-email-vgoyal@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Tue, 05 Jul 2016 15:51:48 +0000 (UTC) Sender: owner-linux-security-module@vger.kernel.org Precedence: bulk List-ID: X-Virus-Scanned: ClamAV using ClamSMTP ovl_getxattr() currently uses vfs_getxattr() on realinode. This fails if mounter does not have DAC/MAC permission to access getxattr. Specifically this becomes a problem when selinux is trying to initialize overlay inode and does ->getxattr(overlay_inode). A task might trigger initialization of overlay inode and we will access real inode xattr in the context of mounter and if mounter does not have permissions, then inode selinux context initialization fails and inode is labeled as unlabeled_t. One way to deal with it is to let SELinux do getxattr checks both on overlay inode and underlying inode and overlay can call vfs_getxattr_noperm() to make sure when selinux is trying to initialize label on inode, it does not go through checks on lower levels and initialization is successful. And after inode initialization, SELinux will make sure task has getatttr permission. One issue with this approach is that it does not work for directories as d_real() returns the overlay dentry for directories and not the underlying directory dentry. Another way to deal with it to introduce another function pointer in inode_operations, say getxattr_noperm(), which is responsible to get xattr without any checks. SELinux initialization code will call this first if it is available on inode. So user space code path will call ->getxattr() and that will go through checks and SELinux internal initialization will call ->getxattr_noperm() and that will not go through checks. For now, I am just converting ovl_getxattr() to get xattr without any checks on underlying inode. That means it is possible for a task to get xattr of a file/dir on lower/upper through overlay mount while it is not possible outside overlay mount. If this is a major concern, I can look into implementing getxattr_noperm(). Signed-off-by: Vivek Goyal --- fs/overlayfs/inode.c | 7 +------ fs/xattr.c | 28 +++++++++++++++++++--------- include/linux/xattr.h | 1 + 3 files changed, 21 insertions(+), 15 deletions(-) diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c index 36dfd86..a5d3320 100644 --- a/fs/overlayfs/inode.c +++ b/fs/overlayfs/inode.c @@ -233,16 +233,11 @@ ssize_t ovl_getxattr(struct dentry *dentry, struct inode *inode, const char *name, void *value, size_t size) { struct dentry *realdentry = ovl_dentry_real(dentry); - ssize_t sz; - const struct cred *old_cred; if (ovl_is_private_xattr(name)) return -ENODATA; - old_cred = ovl_override_creds(dentry->d_sb); - sz = vfs_getxattr(realdentry, name, value, size); - revert_creds(old_cred); - return size; + return vfs_getxattr_noperm(realdentry, name, value, size); } ssize_t ovl_listxattr(struct dentry *dentry, char *list, size_t size) diff --git a/fs/xattr.c b/fs/xattr.c index 4beafc4..973e18c 100644 --- a/fs/xattr.c +++ b/fs/xattr.c @@ -209,19 +209,11 @@ vfs_getxattr_alloc(struct dentry *dentry, const char *name, char **xattr_value, } ssize_t -vfs_getxattr(struct dentry *dentry, const char *name, void *value, size_t size) +vfs_getxattr_noperm(struct dentry *dentry, const char *name, void *value, size_t size) { struct inode *inode = dentry->d_inode; int error; - error = xattr_permission(inode, name, MAY_READ); - if (error) - return error; - - error = security_inode_getxattr(dentry, name); - if (error) - return error; - if (!strncmp(name, XATTR_SECURITY_PREFIX, XATTR_SECURITY_PREFIX_LEN)) { const char *suffix = name + XATTR_SECURITY_PREFIX_LEN; @@ -242,6 +234,24 @@ nolsm: return error; } +EXPORT_SYMBOL_GPL(vfs_getxattr_noperm); + +ssize_t +vfs_getxattr(struct dentry *dentry, const char *name, void *value, size_t size) +{ + struct inode *inode = dentry->d_inode; + int error; + + error = xattr_permission(inode, name, MAY_READ); + if (error) + return error; + + error = security_inode_getxattr(dentry, name); + if (error) + return error; + + return vfs_getxattr_noperm(dentry, name, value, size); +} EXPORT_SYMBOL_GPL(vfs_getxattr); ssize_t diff --git a/include/linux/xattr.h b/include/linux/xattr.h index 94079ba..832a6b6 100644 --- a/include/linux/xattr.h +++ b/include/linux/xattr.h @@ -47,6 +47,7 @@ struct xattr { ssize_t xattr_getsecurity(struct inode *, const char *, void *, size_t); ssize_t vfs_getxattr(struct dentry *, const char *, void *, size_t); +ssize_t vfs_getxattr_noperm(struct dentry *, const char *, void *, size_t); ssize_t vfs_listxattr(struct dentry *d, char *list, size_t size); int __vfs_setxattr_noperm(struct dentry *, const char *, const void *, size_t, int); int vfs_setxattr(struct dentry *, const char *, const void *, size_t, int);