[PATCH v6 0/2]

[PATCH v6 0/2]

[Mark Salyzyn]

overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh

Assumption never checked, should fail if the mounter creds are not
sufficient.

overlayfs: override_creds=off option bypass creator_cred

By default, all access to the upper, lower and work directories is the
recorded mounter's MAC and DAC credentials.  The incoming accesses are
checked against the caller's credentials.

If the principles of least privilege are applied, the mounter's
credentials might not overlap the credentials of the caller's when
accessing the overlayfs filesystem.  For example, a file that a lower
DAC privileged caller can execute, is MAC denied to the generally
higher DAC privileged mounter, to prevent an attack vector.

We add the option to turn off override_creds in the mount options; all
subsequent operations after mount on the filesystem will be only the
caller's credentials.  The module boolean parameter and mount option
override_creds is also added as a presence check for this "feature",
existence of /sys/module/overlay/parameters/overlay_creds

Signed-off-by: Mark Salyzyn <salyzyn@android.com>
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Amir Goldstein <amir73il@gmail.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Stephen Smalley <sds@tycho.nsa.gov>
Cc: linux-unionfs@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

v2:
- Forward port changed attr to stat, resulting in a build error.
- altered commit message.

v3:
- Change name from caller_credentials / creator_credentials to the
  boolean override_creds.
- Changed from creator to mounter credentials.
- Updated and fortified the documentation.
- Added CONFIG_OVERLAY_FS_OVERRIDE_CREDS

v4:
- spelling and grammar errors in text

v5:
- beefed up the caveats in the Documentation
- Is dependent on
  "overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh"
  "overlayfs: check CAP_MKNOD before issuing vfs_whiteout"
- Added prwarn when override_creds=off

v6:
- Drop CONFIG_OVERLAY_FS_OVERRIDE_CREDS.
- Do better with the documentation, drop rationalizations.
- pr_warn message adjusted to report consequences.

[PATCH v6 1/2] overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh

[Mark Salyzyn]

Assumption never checked, should fail if the mounter creds are not
sufficient.

Signed-off-by: Mark Salyzyn <salyzyn@android.com>
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Amir Goldstein <amir73il@gmail.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Stephen Smalley <sds@tycho.nsa.gov>
Cc: linux-unionfs@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: kernel-team@android.com

v5:
- dependency of "overlayfs: override_creds=off option bypass creator_cred"

v6:
- rebase
---
 fs/overlayfs/namei.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index efd372312ef1..aa012b6bd46e 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -163,6 +163,11 @@ struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt,
 	if (!uuid_equal(&fh->uuid, &mnt->mnt_sb->s_uuid))
 		return NULL;
 
+	if (!capable(CAP_DAC_READ_SEARCH)) {
+		origin = ERR_PTR(-EPERM);
+		goto out;
+	}
+
 	bytes = (fh->len - offsetof(struct ovl_fh, fid));
 	real = exportfs_decode_fh(mnt, (struct fid *)fh->fid,
 				  bytes >> 2, (int)fh->type,
-- 
2.19.1.930.g4563a0d9d0-goog

Re: [PATCH v6 1/2] overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh

[Amir Goldstein]

On Mon, Nov 5, 2018 at 8:22 PM Mark Salyzyn <salyzyn@android.com> wrote:
>
> Assumption never checked, should fail if the mounter creds are not
> sufficient.
>
> Signed-off-by: Mark Salyzyn <salyzyn@android.com>
> Cc: Miklos Szeredi <miklos@szeredi.hu>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Vivek Goyal <vgoyal@redhat.com>
> Cc: Eric W. Biederman <ebiederm@xmission.com>
> Cc: Amir Goldstein <amir73il@gmail.com>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Stephen Smalley <sds@tycho.nsa.gov>
> Cc: linux-unionfs@vger.kernel.org
> Cc: linux-doc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: kernel-team@android.com
>
> v5:
> - dependency of "overlayfs: override_creds=off option bypass creator_cred"
>
> v6:
> - rebase

1. rebase onto which branch? doesn't look like the right one
2. pls keep patch revision outside of commit message
    you can put it after --- line
3. revisions are usually ordered latest on the top

> ---
>  fs/overlayfs/namei.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index efd372312ef1..aa012b6bd46e 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -163,6 +163,11 @@ struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt,
>         if (!uuid_equal(&fh->uuid, &mnt->mnt_sb->s_uuid))
>                 return NULL;
>
> +       if (!capable(CAP_DAC_READ_SEARCH)) {
> +               origin = ERR_PTR(-EPERM);
> +               goto out;

I have a deja vu
there is no out label in this function in upstream kernel
you must post patches that are applies and tested on upstream kernel

Thanks,
Amir.

> +       }
> +
>         bytes = (fh->len - offsetof(struct ovl_fh, fid));
>         real = exportfs_decode_fh(mnt, (struct fid *)fh->fid,
>                                   bytes >> 2, (int)fh->type,
> --
> 2.19.1.930.g4563a0d9d0-goog
>

Re: [PATCH v6 1/2] overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2711 bytes --]

Hi Mark,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on miklos-vfs/overlayfs-next]
[also build test ERROR on v4.20-rc1 next-20181105]
[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/overlayfs-check-CAP_DAC_READ_SEARCH-before-issuing-exportfs_decode_fh/20181106-073038
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git overlayfs-next
config: i386-randconfig-x002-201844 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   fs/overlayfs/namei.c: In function 'ovl_decode_real_fh':
>> fs/overlayfs/namei.c:167:3: error: 'origin' undeclared (first use in this function); did you mean 'orig_ist'?
      origin = ERR_PTR(-EPERM);
      ^~~~~~
      orig_ist
   fs/overlayfs/namei.c:167:3: note: each undeclared identifier is reported only once for each function it appears in
>> fs/overlayfs/namei.c:168:3: error: label 'out' used but not defined
      goto out;
      ^~~~

vim +167 fs/overlayfs/namei.c

   152	
   153	struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt,
   154					  bool connected)
   155	{
   156		struct dentry *real;
   157		int bytes;
   158	
   159		/*
   160		 * Make sure that the stored uuid matches the uuid of the lower
   161		 * layer where file handle will be decoded.
   162		 */
   163		if (!uuid_equal(&fh->uuid, &mnt->mnt_sb->s_uuid))
   164			return NULL;
   165	
   166		if (!capable(CAP_DAC_READ_SEARCH)) {
 > 167			origin = ERR_PTR(-EPERM);
 > 168			goto out;
   169		}
   170	
   171		bytes = (fh->len - offsetof(struct ovl_fh, fid));
   172		real = exportfs_decode_fh(mnt, (struct fid *)fh->fid,
   173					  bytes >> 2, (int)fh->type,
   174					  connected ? ovl_acceptable : NULL, mnt);
   175		if (IS_ERR(real)) {
   176			/*
   177			 * Treat stale file handle to lower file as "origin unknown".
   178			 * upper file handle could become stale when upper file is
   179			 * unlinked and this information is needed to handle stale
   180			 * index entries correctly.
   181			 */
   182			if (real == ERR_PTR(-ESTALE) &&
   183			    !(fh->flags & OVL_FH_FLAG_PATH_UPPER))
   184				real = NULL;
   185			return real;
   186		}
   187	
   188		if (ovl_dentry_weird(real)) {
   189			dput(real);
   190			return NULL;
   191		}
   192	
   193		return real;
   194	}
   195	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32369 bytes --]

Re: [PATCH v6 1/2] overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2711 bytes --]

Hi Mark,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on miklos-vfs/overlayfs-next]
[also build test ERROR on v4.20-rc1 next-20181105]
[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/overlayfs-check-CAP_DAC_READ_SEARCH-before-issuing-exportfs_decode_fh/20181106-073038
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git overlayfs-next
config: i386-randconfig-x002-201844 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   fs/overlayfs/namei.c: In function 'ovl_decode_real_fh':
>> fs/overlayfs/namei.c:167:3: error: 'origin' undeclared (first use in this function); did you mean 'orig_ist'?
      origin = ERR_PTR(-EPERM);
      ^~~~~~
      orig_ist
   fs/overlayfs/namei.c:167:3: note: each undeclared identifier is reported only once for each function it appears in
>> fs/overlayfs/namei.c:168:3: error: label 'out' used but not defined
      goto out;
      ^~~~

vim +167 fs/overlayfs/namei.c

   152	
   153	struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt,
   154					  bool connected)
   155	{
   156		struct dentry *real;
   157		int bytes;
   158	
   159		/*
   160		 * Make sure that the stored uuid matches the uuid of the lower
   161		 * layer where file handle will be decoded.
   162		 */
   163		if (!uuid_equal(&fh->uuid, &mnt->mnt_sb->s_uuid))
   164			return NULL;
   165	
   166		if (!capable(CAP_DAC_READ_SEARCH)) {
 > 167			origin = ERR_PTR(-EPERM);
 > 168			goto out;
   169		}
   170	
   171		bytes = (fh->len - offsetof(struct ovl_fh, fid));
   172		real = exportfs_decode_fh(mnt, (struct fid *)fh->fid,
   173					  bytes >> 2, (int)fh->type,
   174					  connected ? ovl_acceptable : NULL, mnt);
   175		if (IS_ERR(real)) {
   176			/*
   177			 * Treat stale file handle to lower file as "origin unknown".
   178			 * upper file handle could become stale when upper file is
   179			 * unlinked and this information is needed to handle stale
   180			 * index entries correctly.
   181			 */
   182			if (real == ERR_PTR(-ESTALE) &&
   183			    !(fh->flags & OVL_FH_FLAG_PATH_UPPER))
   184				real = NULL;
   185			return real;
   186		}
   187	
   188		if (ovl_dentry_weird(real)) {
   189			dput(real);
   190			return NULL;
   191		}
   192	
   193		return real;
   194	}
   195	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32369 bytes --]

Re: [PATCH v6 1/2] overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2879 bytes --]

Hi Mark,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on miklos-vfs/overlayfs-next]
[also build test ERROR on v4.20-rc1 next-20181105]
[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/overlayfs-check-CAP_DAC_READ_SEARCH-before-issuing-exportfs_decode_fh/20181106-073038
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git overlayfs-next
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.2.0 make.cross ARCH=sh 

All errors (new ones prefixed by >>):

   fs//overlayfs/namei.c: In function 'ovl_decode_real_fh':
>> fs//overlayfs/namei.c:167:3: error: 'origin' undeclared (first use in this function); did you mean 'isodigit'?
      origin = ERR_PTR(-EPERM);
      ^~~~~~
      isodigit
   fs//overlayfs/namei.c:167:3: note: each undeclared identifier is reported only once for each function it appears in
   fs//overlayfs/namei.c:168:3: error: label 'out' used but not defined
      goto out;
      ^~~~

vim +167 fs//overlayfs/namei.c

   152	
   153	struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt,
   154					  bool connected)
   155	{
   156		struct dentry *real;
   157		int bytes;
   158	
   159		/*
   160		 * Make sure that the stored uuid matches the uuid of the lower
   161		 * layer where file handle will be decoded.
   162		 */
   163		if (!uuid_equal(&fh->uuid, &mnt->mnt_sb->s_uuid))
   164			return NULL;
   165	
   166		if (!capable(CAP_DAC_READ_SEARCH)) {
 > 167			origin = ERR_PTR(-EPERM);
   168			goto out;
   169		}
   170	
   171		bytes = (fh->len - offsetof(struct ovl_fh, fid));
   172		real = exportfs_decode_fh(mnt, (struct fid *)fh->fid,
   173					  bytes >> 2, (int)fh->type,
   174					  connected ? ovl_acceptable : NULL, mnt);
   175		if (IS_ERR(real)) {
   176			/*
   177			 * Treat stale file handle to lower file as "origin unknown".
   178			 * upper file handle could become stale when upper file is
   179			 * unlinked and this information is needed to handle stale
   180			 * index entries correctly.
   181			 */
   182			if (real == ERR_PTR(-ESTALE) &&
   183			    !(fh->flags & OVL_FH_FLAG_PATH_UPPER))
   184				real = NULL;
   185			return real;
   186		}
   187	
   188		if (ovl_dentry_weird(real)) {
   189			dput(real);
   190			return NULL;
   191		}
   192	
   193		return real;
   194	}
   195	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 49657 bytes --]

Re: [PATCH v6 1/2] overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2879 bytes --]

Hi Mark,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on miklos-vfs/overlayfs-next]
[also build test ERROR on v4.20-rc1 next-20181105]
[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/overlayfs-check-CAP_DAC_READ_SEARCH-before-issuing-exportfs_decode_fh/20181106-073038
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git overlayfs-next
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.2.0 make.cross ARCH=sh 

All errors (new ones prefixed by >>):

   fs//overlayfs/namei.c: In function 'ovl_decode_real_fh':
>> fs//overlayfs/namei.c:167:3: error: 'origin' undeclared (first use in this function); did you mean 'isodigit'?
      origin = ERR_PTR(-EPERM);
      ^~~~~~
      isodigit
   fs//overlayfs/namei.c:167:3: note: each undeclared identifier is reported only once for each function it appears in
   fs//overlayfs/namei.c:168:3: error: label 'out' used but not defined
      goto out;
      ^~~~

vim +167 fs//overlayfs/namei.c

   152	
   153	struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt,
   154					  bool connected)
   155	{
   156		struct dentry *real;
   157		int bytes;
   158	
   159		/*
   160		 * Make sure that the stored uuid matches the uuid of the lower
   161		 * layer where file handle will be decoded.
   162		 */
   163		if (!uuid_equal(&fh->uuid, &mnt->mnt_sb->s_uuid))
   164			return NULL;
   165	
   166		if (!capable(CAP_DAC_READ_SEARCH)) {
 > 167			origin = ERR_PTR(-EPERM);
   168			goto out;
   169		}
   170	
   171		bytes = (fh->len - offsetof(struct ovl_fh, fid));
   172		real = exportfs_decode_fh(mnt, (struct fid *)fh->fid,
   173					  bytes >> 2, (int)fh->type,
   174					  connected ? ovl_acceptable : NULL, mnt);
   175		if (IS_ERR(real)) {
   176			/*
   177			 * Treat stale file handle to lower file as "origin unknown".
   178			 * upper file handle could become stale when upper file is
   179			 * unlinked and this information is needed to handle stale
   180			 * index entries correctly.
   181			 */
   182			if (real == ERR_PTR(-ESTALE) &&
   183			    !(fh->flags & OVL_FH_FLAG_PATH_UPPER))
   184				real = NULL;
   185			return real;
   186		}
   187	
   188		if (ovl_dentry_weird(real)) {
   189			dput(real);
   190			return NULL;
   191		}
   192	
   193		return real;
   194	}
   195	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 49657 bytes --]

[PATCH v6 2/2] overlayfs: override_creds=off option bypass creator_cred

[Mark Salyzyn]

By default, all access to the upper, lower and work directories is the
recorded mounter's MAC and DAC credentials.  The incoming accesses are
checked against the caller's credentials.

If the principles of least privilege are applied, the mounter's
credentials might not overlap the credentials of the caller's when
accessing the overlayfs filesystem.  For example, a file that a lower
DAC privileged caller can execute, is MAC denied to the generally
higher DAC privileged mounter, to prevent an attack vector.

We add the option to turn off override_creds in the mount options; all
subsequent operations after mount on the filesystem will be only the
caller's credentials.  The module boolean parameter and mount option
override_creds is also added as a presence check for this "feature",
existence of /sys/module/overlay/parameters/overlay_creds.

Signed-off-by: Mark Salyzyn <salyzyn@android.com>
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Amir Goldstein <amir73il@gmail.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Stephen Smalley <sds@tycho.nsa.gov>
Cc: linux-unionfs@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: kernel-team@android.com

v2:
- Forward port changed attr to stat, resulting in a build error.
- altered commit message.

v3:
- Change name from caller_credentials / creator_credentials to the
  boolean override_creds.
- Changed from creator to mounter credentials.
- Updated and fortified the documentation.
- Added CONFIG_OVERLAY_FS_OVERRIDE_CREDS

v4:
- spelling and grammar errors in text

v5:
- beefed up the caveats in the Documentation
- Is dependent on
  "overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh"
  "overlayfs: check CAP_MKNOD before issuing vfs_whiteout"
- Added prwarn when override_creds=off

v6:
- Drop CONFIG_OVERLAY_FS_OVERRIDE_CREDS.
- Do better with the documentation.
- pr_warn message adjusted to report consequences.
---
 Documentation/filesystems/overlayfs.txt | 17 +++++++++++++++++
 fs/overlayfs/copy_up.c                  |  2 +-
 fs/overlayfs/dir.c                      |  9 +++++----
 fs/overlayfs/inode.c                    | 16 ++++++++--------
 fs/overlayfs/namei.c                    |  6 +++---
 fs/overlayfs/overlayfs.h                |  1 +
 fs/overlayfs/ovl_entry.h                |  1 +
 fs/overlayfs/readdir.c                  |  4 ++--
 fs/overlayfs/super.c                    | 23 ++++++++++++++++++++++-
 fs/overlayfs/util.c                     | 12 ++++++++++--
 10 files changed, 70 insertions(+), 21 deletions(-)

diff --git a/Documentation/filesystems/overlayfs.txt b/Documentation/filesystems/overlayfs.txt
index eef7d9d259e8..5cc299df4436 100644
--- a/Documentation/filesystems/overlayfs.txt
+++ b/Documentation/filesystems/overlayfs.txt
@@ -102,6 +102,23 @@ Only the lists of names from directories are merged.  Other content
 such as metadata and extended attributes are reported for the upper
 directory only.  These attributes of the lower directory are hidden.
 
+credentials
+-----------
+
+By default, all access to the upper, lower and work directories is the
+recorded mounter's MAC and DAC credentials.  The incoming accesses are
+checked against the caller's credentials.
+
+override_creds mount flag turned off is reserved for when mounter and
+caller MAC or DAC credentials do not overlap.  Several unintended side
+effects will occur.  The caller with a lower privilege will not be
+able to delete files or directories, create nodes, or search some
+directories.  The caller with higher privilege can perform unexpected
+or unsecured operations.  The uneven security model where upperdir
+and workdir are opened at privilege, but accessed without, should only
+be used with strict understanding of the side effects and of the
+security policies.
+
 whiteouts and opaque directories
 --------------------------------
 
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 9e62dcf06fc4..dfab62ce7504 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -860,7 +860,7 @@ int ovl_copy_up_flags(struct dentry *dentry, int flags)
 		dput(parent);
 		dput(next);
 	}
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 
 	return err;
 }
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index c6289147c787..b7052e23c467 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -566,7 +566,8 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
 		override_cred->fsgid = inode->i_gid;
 		if (!attr->hardlink) {
 			err = security_dentry_create_files_as(dentry,
-					attr->mode, &dentry->d_name, old_cred,
+					attr->mode, &dentry->d_name,
+					old_cred ? old_cred : current_cred(),
 					override_cred);
 			if (err) {
 				put_cred(override_cred);
@@ -582,7 +583,7 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
 			err = ovl_create_over_whiteout(dentry, inode, attr);
 	}
 out_revert_creds:
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 	return err;
 }
 
@@ -842,7 +843,7 @@ static int ovl_do_remove(struct dentry *dentry, bool is_dir)
 		err = ovl_remove_upper(dentry, is_dir, &list);
 	else
 		err = ovl_remove_and_whiteout(dentry, &list);
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 	if (!err) {
 		if (is_dir)
 			clear_nlink(dentry->d_inode);
@@ -1212,7 +1213,7 @@ static int ovl_rename(struct inode *olddir, struct dentry *old,
 out_unlock:
 	unlock_rename(new_upperdir, old_upperdir);
 out_revert_creds:
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 	if (update_nlink)
 		ovl_nlink_end(new);
 out_drop_write:
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 6bcc9dedc342..192f5508ed45 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -64,7 +64,7 @@ int ovl_setattr(struct dentry *dentry, struct iattr *attr)
 		inode_lock(upperdentry->d_inode);
 		old_cred = ovl_override_creds(dentry->d_sb);
 		err = notify_change(upperdentry, attr, NULL);
-		revert_creds(old_cred);
+		ovl_revert_creds(old_cred);
 		if (!err)
 			ovl_copyattr(upperdentry->d_inode, dentry->d_inode);
 		inode_unlock(upperdentry->d_inode);
@@ -260,7 +260,7 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
 		stat->nlink = dentry->d_inode->i_nlink;
 
 out:
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 
 	return err;
 }
@@ -303,7 +303,7 @@ int ovl_permission(struct inode *inode, int mask)
 
 	old_cred = ovl_override_creds(inode->i_sb);
 	err = inode_permission(realinode, mask);
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 
 	return err;
 }
@@ -320,7 +320,7 @@ static const char *ovl_get_link(struct dentry *dentry,
 
 	old_cred = ovl_override_creds(dentry->d_sb);
 	p = vfs_get_link(ovl_dentry_real(dentry), done);
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 	return p;
 }
 
@@ -363,7 +363,7 @@ int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char *name,
 		WARN_ON(flags != XATTR_REPLACE);
 		err = vfs_removexattr(realdentry, name);
 	}
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 
 	/* copy c/mtime */
 	ovl_copyattr(d_inode(realdentry), inode);
@@ -384,7 +384,7 @@ int ovl_xattr_get(struct dentry *dentry, struct inode *inode, const char *name,
 
 	old_cred = ovl_override_creds(dentry->d_sb);
 	res = vfs_getxattr(realdentry, name, value, size);
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 	return res;
 }
 
@@ -408,7 +408,7 @@ ssize_t ovl_listxattr(struct dentry *dentry, char *list, size_t size)
 
 	old_cred = ovl_override_creds(dentry->d_sb);
 	res = vfs_listxattr(realdentry, list, size);
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 	if (res <= 0 || size == 0)
 		return res;
 
@@ -443,7 +443,7 @@ struct posix_acl *ovl_get_acl(struct inode *inode, int type)
 
 	old_cred = ovl_override_creds(inode->i_sb);
 	acl = get_acl(realinode, type);
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 
 	return acl;
 }
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index aa012b6bd46e..b73e5f7aea2e 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -1074,7 +1074,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			goto out_free_oe;
 	}
 
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 	if (origin_path) {
 		dput(origin_path->dentry);
 		kfree(origin_path);
@@ -1101,7 +1101,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	kfree(upperredirect);
 out:
 	kfree(d.redirect);
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 	return ERR_PTR(err);
 }
 
@@ -1155,7 +1155,7 @@ bool ovl_lower_positive(struct dentry *dentry)
 			dput(this);
 		}
 	}
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 
 	return positive;
 }
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 5e45cb3630a0..6f8b6f9ff357 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -208,6 +208,7 @@ int ovl_want_write(struct dentry *dentry);
 void ovl_drop_write(struct dentry *dentry);
 struct dentry *ovl_workdir(struct dentry *dentry);
 const struct cred *ovl_override_creds(struct super_block *sb);
+void ovl_revert_creds(const struct cred *oldcred);
 struct super_block *ovl_same_sb(struct super_block *sb);
 int ovl_can_decode_fh(struct super_block *sb);
 struct dentry *ovl_indexdir(struct super_block *sb);
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index ec237035333a..e38eea8104be 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -20,6 +20,7 @@ struct ovl_config {
 	bool nfs_export;
 	int xino;
 	bool metacopy;
+	bool override_creds;
 };
 
 struct ovl_sb {
diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index cc8303a806b4..ec591b49e902 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -289,7 +289,7 @@ static int ovl_check_whiteouts(struct dentry *dir, struct ovl_readdir_data *rdd)
 		}
 		inode_unlock(dir->d_inode);
 	}
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 
 	return err;
 }
@@ -921,7 +921,7 @@ int ovl_check_empty_dir(struct dentry *dentry, struct list_head *list)
 
 	old_cred = ovl_override_creds(dentry->d_sb);
 	err = ovl_dir_read_merged(dentry, list, &root);
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 	if (err)
 		return err;
 
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 0116735cc321..1669d4fa7ad8 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -56,6 +56,11 @@ module_param_named(xino_auto, ovl_xino_auto_def, bool, 0644);
 MODULE_PARM_DESC(ovl_xino_auto_def,
 		 "Auto enable xino feature");
 
+static bool __read_mostly ovl_default_override_creds = true;
+module_param_named(override_creds, ovl_default_override_creds, bool, 0644);
+MODULE_PARM_DESC(ovl_default_override_creds,
+		 "Use mounter's credentials for accesses");
+
 static void ovl_entry_stack_free(struct ovl_entry *oe)
 {
 	unsigned int i;
@@ -362,6 +367,8 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
 	if (ofs->config.metacopy != ovl_metacopy_def)
 		seq_printf(m, ",metacopy=%s",
 			   ofs->config.metacopy ? "on" : "off");
+	seq_show_option(m, "override_creds",
+			ofs->config.override_creds ? "on" : "off");
 	return 0;
 }
 
@@ -401,6 +408,8 @@ enum {
 	OPT_XINO_AUTO,
 	OPT_METACOPY_ON,
 	OPT_METACOPY_OFF,
+	OPT_OVERRIDE_CREDS_ON,
+	OPT_OVERRIDE_CREDS_OFF,
 	OPT_ERR,
 };
 
@@ -419,6 +428,8 @@ static const match_table_t ovl_tokens = {
 	{OPT_XINO_AUTO,			"xino=auto"},
 	{OPT_METACOPY_ON,		"metacopy=on"},
 	{OPT_METACOPY_OFF,		"metacopy=off"},
+	{OPT_OVERRIDE_CREDS_ON,		"override_creds=on"},
+	{OPT_OVERRIDE_CREDS_OFF,	"override_creds=off"},
 	{OPT_ERR,			NULL}
 };
 
@@ -477,6 +488,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 	config->redirect_mode = kstrdup(ovl_redirect_mode_def(), GFP_KERNEL);
 	if (!config->redirect_mode)
 		return -ENOMEM;
+	config->override_creds = ovl_default_override_creds;
 
 	while ((p = ovl_next_opt(&opt)) != NULL) {
 		int token;
@@ -557,6 +569,14 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 			config->metacopy = false;
 			break;
 
+		case OPT_OVERRIDE_CREDS_ON:
+			config->override_creds = true;
+			break;
+
+		case OPT_OVERRIDE_CREDS_OFF:
+			config->override_creds = false;
+			break;
+
 		default:
 			pr_err("overlayfs: unrecognized mount option \"%s\" or missing value\n", p);
 			return -EINVAL;
@@ -1549,7 +1569,8 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 		       ovl_dentry_lower(root_dentry), NULL);
 
 	sb->s_root = root_dentry;
-
+	if (!ofs->config.override_creds)
+		pr_warn("overlayfs: override_creds=off, caller credentials may not be enough to delete file or directories, create nodes, or search directories.\n");
 	return 0;
 
 out_free_oe:
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 7c01327b1852..484d7f76ac9c 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -40,9 +40,17 @@ const struct cred *ovl_override_creds(struct super_block *sb)
 {
 	struct ovl_fs *ofs = sb->s_fs_info;
 
+	if (!ofs->config.override_creds)
+		return NULL;
 	return override_creds(ofs->creator_cred);
 }
 
+void ovl_revert_creds(const struct cred *old_cred)
+{
+	if (old_cred)
+		revert_creds(old_cred);
+}
+
 struct super_block *ovl_same_sb(struct super_block *sb)
 {
 	struct ovl_fs *ofs = sb->s_fs_info;
@@ -782,7 +790,7 @@ int ovl_nlink_start(struct dentry *dentry)
 	 * value relative to the upper inode nlink in an upper inode xattr.
 	 */
 	err = ovl_set_nlink_upper(dentry);
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 
 out:
 	if (err)
@@ -800,7 +808,7 @@ void ovl_nlink_end(struct dentry *dentry)
 
 		old_cred = ovl_override_creds(dentry->d_sb);
 		ovl_cleanup_index(dentry);
-		revert_creds(old_cred);
+		ovl_revert_creds(old_cred);
 	}
 
 	ovl_inode_unlock(inode);
-- 
2.19.1.930.g4563a0d9d0-goog

Re: [PATCH v6 2/2] overlayfs: override_creds=off option bypass creator_cred

[Amir Goldstein]

On Mon, Nov 5, 2018 at 8:22 PM Mark Salyzyn <salyzyn@android.com> wrote:
>
> By default, all access to the upper, lower and work directories is the
> recorded mounter's MAC and DAC credentials.  The incoming accesses are
> checked against the caller's credentials.
>
> If the principles of least privilege are applied, the mounter's
> credentials might not overlap the credentials of the caller's when
> accessing the overlayfs filesystem.  For example, a file that a lower
> DAC privileged caller can execute, is MAC denied to the generally
> higher DAC privileged mounter, to prevent an attack vector.
>
> We add the option to turn off override_creds in the mount options; all
> subsequent operations after mount on the filesystem will be only the
> caller's credentials.  The module boolean parameter and mount option
> override_creds is also added as a presence check for this "feature",
> existence of /sys/module/overlay/parameters/overlay_creds.
>
> Signed-off-by: Mark Salyzyn <salyzyn@android.com>
> Cc: Miklos Szeredi <miklos@szeredi.hu>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Vivek Goyal <vgoyal@redhat.com>
> Cc: Eric W. Biederman <ebiederm@xmission.com>
> Cc: Amir Goldstein <amir73il@gmail.com>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Stephen Smalley <sds@tycho.nsa.gov>
> Cc: linux-unionfs@vger.kernel.org
> Cc: linux-doc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: kernel-team@android.com
>
> v2:
> - Forward port changed attr to stat, resulting in a build error.
> - altered commit message.
>
> v3:
> - Change name from caller_credentials / creator_credentials to the
>   boolean override_creds.
> - Changed from creator to mounter credentials.
> - Updated and fortified the documentation.
> - Added CONFIG_OVERLAY_FS_OVERRIDE_CREDS
>
> v4:
> - spelling and grammar errors in text
>
> v5:
> - beefed up the caveats in the Documentation
> - Is dependent on
>   "overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh"
>   "overlayfs: check CAP_MKNOD before issuing vfs_whiteout"
> - Added prwarn when override_creds=off
>
> v6:
> - Drop CONFIG_OVERLAY_FS_OVERRIDE_CREDS.
> - Do better with the documentation.
> - pr_warn message adjusted to report consequences.

same comment about patch revision - not in commit message.

> ---
>  Documentation/filesystems/overlayfs.txt | 17 +++++++++++++++++
>  fs/overlayfs/copy_up.c                  |  2 +-
>  fs/overlayfs/dir.c                      |  9 +++++----
>  fs/overlayfs/inode.c                    | 16 ++++++++--------
>  fs/overlayfs/namei.c                    |  6 +++---
>  fs/overlayfs/overlayfs.h                |  1 +
>  fs/overlayfs/ovl_entry.h                |  1 +
>  fs/overlayfs/readdir.c                  |  4 ++--
>  fs/overlayfs/super.c                    | 23 ++++++++++++++++++++++-
>  fs/overlayfs/util.c                     | 12 ++++++++++--
>  10 files changed, 70 insertions(+), 21 deletions(-)
>
> diff --git a/Documentation/filesystems/overlayfs.txt b/Documentation/filesystems/overlayfs.txt
> index eef7d9d259e8..5cc299df4436 100644
> --- a/Documentation/filesystems/overlayfs.txt
> +++ b/Documentation/filesystems/overlayfs.txt
> @@ -102,6 +102,23 @@ Only the lists of names from directories are merged.  Other content
>  such as metadata and extended attributes are reported for the upper
>  directory only.  These attributes of the lower directory are hidden.
>
> +credentials
> +-----------
> +
> +By default, all access to the upper, lower and work directories is the
> +recorded mounter's MAC and DAC credentials.  The incoming accesses are
> +checked against the caller's credentials.
> +
> +override_creds mount flag turned off is reserved for when mounter and
> +caller MAC or DAC credentials do not overlap.  Several unintended side
> +effects will occur.  The caller with a lower privilege will not be
> +able to delete files or directories, create nodes, or search some
> +directories.  The caller with higher privilege can perform unexpected
> +or unsecured operations.  The uneven security model where upperdir
> +and workdir are opened at privilege, but accessed without, should only
> +be used with strict understanding of the side effects and of the
> +security policies.
> +
>  whiteouts and opaque directories
>  --------------------------------
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 9e62dcf06fc4..dfab62ce7504 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -860,7 +860,7 @@ int ovl_copy_up_flags(struct dentry *dentry, int flags)
>                 dput(parent);
>                 dput(next);
>         }
> -       revert_creds(old_cred);
> +       ovl_revert_creds(old_cred);
>
>         return err;
>  }
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index c6289147c787..b7052e23c467 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -566,7 +566,8 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
>                 override_cred->fsgid = inode->i_gid;
>                 if (!attr->hardlink) {
>                         err = security_dentry_create_files_as(dentry,
> -                                       attr->mode, &dentry->d_name, old_cred,
> +                                       attr->mode, &dentry->d_name,
> +                                       old_cred ? old_cred : current_cred(),
>                                         override_cred);
>                         if (err) {
>                                 put_cred(override_cred);
> @@ -582,7 +583,7 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
>                         err = ovl_create_over_whiteout(dentry, inode, attr);
>         }
>  out_revert_creds:
> -       revert_creds(old_cred);
> +       ovl_revert_creds(old_cred);
>         return err;
>  }
>
> @@ -842,7 +843,7 @@ static int ovl_do_remove(struct dentry *dentry, bool is_dir)
>                 err = ovl_remove_upper(dentry, is_dir, &list);
>         else
>                 err = ovl_remove_and_whiteout(dentry, &list);
> -       revert_creds(old_cred);
> +       ovl_revert_creds(old_cred);
>         if (!err) {
>                 if (is_dir)
>                         clear_nlink(dentry->d_inode);
> @@ -1212,7 +1213,7 @@ static int ovl_rename(struct inode *olddir, struct dentry *old,
>  out_unlock:
>         unlock_rename(new_upperdir, old_upperdir);
>  out_revert_creds:
> -       revert_creds(old_cred);
> +       ovl_revert_creds(old_cred);
>         if (update_nlink)
>                 ovl_nlink_end(new);
>  out_drop_write:
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 6bcc9dedc342..192f5508ed45 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -64,7 +64,7 @@ int ovl_setattr(struct dentry *dentry, struct iattr *attr)
>                 inode_lock(upperdentry->d_inode);
>                 old_cred = ovl_override_creds(dentry->d_sb);
>                 err = notify_change(upperdentry, attr, NULL);
> -               revert_creds(old_cred);
> +               ovl_revert_creds(old_cred);
>                 if (!err)
>                         ovl_copyattr(upperdentry->d_inode, dentry->d_inode);
>                 inode_unlock(upperdentry->d_inode);
> @@ -260,7 +260,7 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
>                 stat->nlink = dentry->d_inode->i_nlink;
>
>  out:
> -       revert_creds(old_cred);
> +       ovl_revert_creds(old_cred);
>
>         return err;
>  }
> @@ -303,7 +303,7 @@ int ovl_permission(struct inode *inode, int mask)
>
>         old_cred = ovl_override_creds(inode->i_sb);
>         err = inode_permission(realinode, mask);
> -       revert_creds(old_cred);
> +       ovl_revert_creds(old_cred);
>
>         return err;
>  }
> @@ -320,7 +320,7 @@ static const char *ovl_get_link(struct dentry *dentry,
>
>         old_cred = ovl_override_creds(dentry->d_sb);
>         p = vfs_get_link(ovl_dentry_real(dentry), done);
> -       revert_creds(old_cred);
> +       ovl_revert_creds(old_cred);
>         return p;
>  }
>
> @@ -363,7 +363,7 @@ int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char *name,
>                 WARN_ON(flags != XATTR_REPLACE);
>                 err = vfs_removexattr(realdentry, name);
>         }
> -       revert_creds(old_cred);
> +       ovl_revert_creds(old_cred);
>
>         /* copy c/mtime */
>         ovl_copyattr(d_inode(realdentry), inode);
> @@ -384,7 +384,7 @@ int ovl_xattr_get(struct dentry *dentry, struct inode *inode, const char *name,
>
>         old_cred = ovl_override_creds(dentry->d_sb);
>         res = vfs_getxattr(realdentry, name, value, size);
> -       revert_creds(old_cred);
> +       ovl_revert_creds(old_cred);
>         return res;
>  }
>
> @@ -408,7 +408,7 @@ ssize_t ovl_listxattr(struct dentry *dentry, char *list, size_t size)
>
>         old_cred = ovl_override_creds(dentry->d_sb);
>         res = vfs_listxattr(realdentry, list, size);
> -       revert_creds(old_cred);
> +       ovl_revert_creds(old_cred);
>         if (res <= 0 || size == 0)
>                 return res;
>
> @@ -443,7 +443,7 @@ struct posix_acl *ovl_get_acl(struct inode *inode, int type)
>
>         old_cred = ovl_override_creds(inode->i_sb);
>         acl = get_acl(realinode, type);
> -       revert_creds(old_cred);
> +       ovl_revert_creds(old_cred);
>
>         return acl;
>  }
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index aa012b6bd46e..b73e5f7aea2e 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -1074,7 +1074,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                         goto out_free_oe;
>         }
>
> -       revert_creds(old_cred);
> +       ovl_revert_creds(old_cred);
>         if (origin_path) {
>                 dput(origin_path->dentry);
>                 kfree(origin_path);
> @@ -1101,7 +1101,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>         kfree(upperredirect);
>  out:
>         kfree(d.redirect);
> -       revert_creds(old_cred);
> +       ovl_revert_creds(old_cred);
>         return ERR_PTR(err);
>  }
>
> @@ -1155,7 +1155,7 @@ bool ovl_lower_positive(struct dentry *dentry)
>                         dput(this);
>                 }
>         }
> -       revert_creds(old_cred);
> +       ovl_revert_creds(old_cred);
>
>         return positive;
>  }
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 5e45cb3630a0..6f8b6f9ff357 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -208,6 +208,7 @@ int ovl_want_write(struct dentry *dentry);
>  void ovl_drop_write(struct dentry *dentry);
>  struct dentry *ovl_workdir(struct dentry *dentry);
>  const struct cred *ovl_override_creds(struct super_block *sb);
> +void ovl_revert_creds(const struct cred *oldcred);
>  struct super_block *ovl_same_sb(struct super_block *sb);
>  int ovl_can_decode_fh(struct super_block *sb);
>  struct dentry *ovl_indexdir(struct super_block *sb);
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index ec237035333a..e38eea8104be 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -20,6 +20,7 @@ struct ovl_config {
>         bool nfs_export;
>         int xino;
>         bool metacopy;
> +       bool override_creds;
>  };
>
>  struct ovl_sb {
> diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> index cc8303a806b4..ec591b49e902 100644
> --- a/fs/overlayfs/readdir.c
> +++ b/fs/overlayfs/readdir.c
> @@ -289,7 +289,7 @@ static int ovl_check_whiteouts(struct dentry *dir, struct ovl_readdir_data *rdd)
>                 }
>                 inode_unlock(dir->d_inode);
>         }
> -       revert_creds(old_cred);
> +       ovl_revert_creds(old_cred);
>
>         return err;
>  }
> @@ -921,7 +921,7 @@ int ovl_check_empty_dir(struct dentry *dentry, struct list_head *list)
>
>         old_cred = ovl_override_creds(dentry->d_sb);
>         err = ovl_dir_read_merged(dentry, list, &root);
> -       revert_creds(old_cred);
> +       ovl_revert_creds(old_cred);
>         if (err)
>                 return err;
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 0116735cc321..1669d4fa7ad8 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -56,6 +56,11 @@ module_param_named(xino_auto, ovl_xino_auto_def, bool, 0644);
>  MODULE_PARM_DESC(ovl_xino_auto_def,
>                  "Auto enable xino feature");
>
> +static bool __read_mostly ovl_default_override_creds = true;

Please stick to conventions - ovl_override_creds_def.

> +module_param_named(override_creds, ovl_default_override_creds, bool, 0644);
> +MODULE_PARM_DESC(ovl_default_override_creds,
> +                "Use mounter's credentials for accesses");
> +
>  static void ovl_entry_stack_free(struct ovl_entry *oe)
>  {
>         unsigned int i;
> @@ -362,6 +367,8 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
>         if (ofs->config.metacopy != ovl_metacopy_def)
>                 seq_printf(m, ",metacopy=%s",
>                            ofs->config.metacopy ? "on" : "off");
> +       seq_show_option(m, "override_creds",

show only if != ovl_override_creds_def

> +                       ofs->config.override_creds ? "on" : "off");
>         return 0;
>  }
>
> @@ -401,6 +408,8 @@ enum {
>         OPT_XINO_AUTO,
>         OPT_METACOPY_ON,
>         OPT_METACOPY_OFF,
> +       OPT_OVERRIDE_CREDS_ON,
> +       OPT_OVERRIDE_CREDS_OFF,
>         OPT_ERR,
>  };
>
> @@ -419,6 +428,8 @@ static const match_table_t ovl_tokens = {
>         {OPT_XINO_AUTO,                 "xino=auto"},
>         {OPT_METACOPY_ON,               "metacopy=on"},
>         {OPT_METACOPY_OFF,              "metacopy=off"},
> +       {OPT_OVERRIDE_CREDS_ON,         "override_creds=on"},
> +       {OPT_OVERRIDE_CREDS_OFF,        "override_creds=off"},
>         {OPT_ERR,                       NULL}
>  };
>
> @@ -477,6 +488,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>         config->redirect_mode = kstrdup(ovl_redirect_mode_def(), GFP_KERNEL);
>         if (!config->redirect_mode)
>                 return -ENOMEM;
> +       config->override_creds = ovl_default_override_creds;
>
>         while ((p = ovl_next_opt(&opt)) != NULL) {
>                 int token;
> @@ -557,6 +569,14 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>                         config->metacopy = false;
>                         break;
>
> +               case OPT_OVERRIDE_CREDS_ON:
> +                       config->override_creds = true;
> +                       break;
> +
> +               case OPT_OVERRIDE_CREDS_OFF:
> +                       config->override_creds = false;
> +                       break;
> +
>                 default:
>                         pr_err("overlayfs: unrecognized mount option \"%s\" or missing value\n", p);
>                         return -EINVAL;
> @@ -1549,7 +1569,8 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>                        ovl_dentry_lower(root_dentry), NULL);
>
>         sb->s_root = root_dentry;
> -
> +       if (!ofs->config.override_creds)
> +               pr_warn("overlayfs: override_creds=off, caller credentials may not be enough to delete file or directories, create nodes, or search directories.\n");

The audience is someone that has this feature on by mistake or someone
that turn it
on without understanding what it does. I am not sure that this is
scary enough, but
I don't have a better suggestion.
Will let others state their opinion.

Thanks,
Amir.

Re: [PATCH v6 2/2] overlayfs: override_creds=off option bypass creator_cred

[Miklos Szeredi]

On Mon, Nov 5, 2018 at 7:47 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Mon, Nov 5, 2018 at 8:22 PM Mark Salyzyn <salyzyn@android.com> wrote:
>>
>> By default, all access to the upper, lower and work directories is the
>> recorded mounter's MAC and DAC credentials.  The incoming accesses are
>> checked against the caller's credentials.
>>
>> If the principles of least privilege are applied, the mounter's
>> credentials might not overlap the credentials of the caller's when
>> accessing the overlayfs filesystem.  For example, a file that a lower
>> DAC privileged caller can execute, is MAC denied to the generally
>> higher DAC privileged mounter, to prevent an attack vector.
>>
>> We add the option to turn off override_creds in the mount options; all
>> subsequent operations after mount on the filesystem will be only the
>> caller's credentials.  The module boolean parameter and mount option
>> override_creds is also added as a presence check for this "feature",
>> existence of /sys/module/overlay/parameters/overlay_creds.
>>
>> Signed-off-by: Mark Salyzyn <salyzyn@android.com>
>> Cc: Miklos Szeredi <miklos@szeredi.hu>
>> Cc: Jonathan Corbet <corbet@lwn.net>
>> Cc: Vivek Goyal <vgoyal@redhat.com>
>> Cc: Eric W. Biederman <ebiederm@xmission.com>
>> Cc: Amir Goldstein <amir73il@gmail.com>
>> Cc: Randy Dunlap <rdunlap@infradead.org>
>> Cc: Stephen Smalley <sds@tycho.nsa.gov>
>> Cc: linux-unionfs@vger.kernel.org
>> Cc: linux-doc@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Cc: kernel-team@android.com
>>
>> v2:
>> - Forward port changed attr to stat, resulting in a build error.
>> - altered commit message.
>>
>> v3:
>> - Change name from caller_credentials / creator_credentials to the
>>   boolean override_creds.
>> - Changed from creator to mounter credentials.
>> - Updated and fortified the documentation.
>> - Added CONFIG_OVERLAY_FS_OVERRIDE_CREDS
>>
>> v4:
>> - spelling and grammar errors in text
>>
>> v5:
>> - beefed up the caveats in the Documentation
>> - Is dependent on
>>   "overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh"
>>   "overlayfs: check CAP_MKNOD before issuing vfs_whiteout"
>> - Added prwarn when override_creds=off
>>
>> v6:
>> - Drop CONFIG_OVERLAY_FS_OVERRIDE_CREDS.
>> - Do better with the documentation.
>> - pr_warn message adjusted to report consequences.
>
> same comment about patch revision - not in commit message.
>
>> ---
>>  Documentation/filesystems/overlayfs.txt | 17 +++++++++++++++++
>>  fs/overlayfs/copy_up.c                  |  2 +-
>>  fs/overlayfs/dir.c                      |  9 +++++----
>>  fs/overlayfs/inode.c                    | 16 ++++++++--------
>>  fs/overlayfs/namei.c                    |  6 +++---
>>  fs/overlayfs/overlayfs.h                |  1 +
>>  fs/overlayfs/ovl_entry.h                |  1 +
>>  fs/overlayfs/readdir.c                  |  4 ++--
>>  fs/overlayfs/super.c                    | 23 ++++++++++++++++++++++-
>>  fs/overlayfs/util.c                     | 12 ++++++++++--
>>  10 files changed, 70 insertions(+), 21 deletions(-)
>>
>> diff --git a/Documentation/filesystems/overlayfs.txt b/Documentation/filesystems/overlayfs.txt
>> index eef7d9d259e8..5cc299df4436 100644
>> --- a/Documentation/filesystems/overlayfs.txt
>> +++ b/Documentation/filesystems/overlayfs.txt
>> @@ -102,6 +102,23 @@ Only the lists of names from directories are merged.  Other content
>>  such as metadata and extended attributes are reported for the upper
>>  directory only.  These attributes of the lower directory are hidden.
>>
>> +credentials
>> +-----------
>> +
>> +By default, all access to the upper, lower and work directories is the
>> +recorded mounter's MAC and DAC credentials.  The incoming accesses are
>> +checked against the caller's credentials.
>> +
>> +override_creds mount flag turned off is reserved for when mounter and
>> +caller MAC or DAC credentials do not overlap.  Several unintended side
>> +effects will occur.  The caller with a lower privilege will not be
>> +able to delete files or directories, create nodes, or search some
>> +directories.  The caller with higher privilege can perform unexpected
>> +or unsecured operations.  The uneven security model where upperdir
>> +and workdir are opened at privilege, but accessed without, should only
>> +be used with strict understanding of the side effects and of the
>> +security policies.
>> +
>>  whiteouts and opaque directories
>>  --------------------------------
>>
>> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
>> index 9e62dcf06fc4..dfab62ce7504 100644
>> --- a/fs/overlayfs/copy_up.c
>> +++ b/fs/overlayfs/copy_up.c
>> @@ -860,7 +860,7 @@ int ovl_copy_up_flags(struct dentry *dentry, int flags)
>>                 dput(parent);
>>                 dput(next);
>>         }
>> -       revert_creds(old_cred);
>> +       ovl_revert_creds(old_cred);
>>
>>         return err;
>>  }
>> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
>> index c6289147c787..b7052e23c467 100644
>> --- a/fs/overlayfs/dir.c
>> +++ b/fs/overlayfs/dir.c
>> @@ -566,7 +566,8 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
>>                 override_cred->fsgid = inode->i_gid;
>>                 if (!attr->hardlink) {
>>                         err = security_dentry_create_files_as(dentry,
>> -                                       attr->mode, &dentry->d_name, old_cred,
>> +                                       attr->mode, &dentry->d_name,
>> +                                       old_cred ? old_cred : current_cred(),
>>                                         override_cred);
>>                         if (err) {
>>                                 put_cred(override_cred);
>> @@ -582,7 +583,7 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
>>                         err = ovl_create_over_whiteout(dentry, inode, attr);
>>         }
>>  out_revert_creds:
>> -       revert_creds(old_cred);
>> +       ovl_revert_creds(old_cred);
>>         return err;
>>  }
>>
>> @@ -842,7 +843,7 @@ static int ovl_do_remove(struct dentry *dentry, bool is_dir)
>>                 err = ovl_remove_upper(dentry, is_dir, &list);
>>         else
>>                 err = ovl_remove_and_whiteout(dentry, &list);
>> -       revert_creds(old_cred);
>> +       ovl_revert_creds(old_cred);
>>         if (!err) {
>>                 if (is_dir)
>>                         clear_nlink(dentry->d_inode);
>> @@ -1212,7 +1213,7 @@ static int ovl_rename(struct inode *olddir, struct dentry *old,
>>  out_unlock:
>>         unlock_rename(new_upperdir, old_upperdir);
>>  out_revert_creds:
>> -       revert_creds(old_cred);
>> +       ovl_revert_creds(old_cred);
>>         if (update_nlink)
>>                 ovl_nlink_end(new);
>>  out_drop_write:
>> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
>> index 6bcc9dedc342..192f5508ed45 100644
>> --- a/fs/overlayfs/inode.c
>> +++ b/fs/overlayfs/inode.c
>> @@ -64,7 +64,7 @@ int ovl_setattr(struct dentry *dentry, struct iattr *attr)
>>                 inode_lock(upperdentry->d_inode);
>>                 old_cred = ovl_override_creds(dentry->d_sb);
>>                 err = notify_change(upperdentry, attr, NULL);
>> -               revert_creds(old_cred);
>> +               ovl_revert_creds(old_cred);
>>                 if (!err)
>>                         ovl_copyattr(upperdentry->d_inode, dentry->d_inode);
>>                 inode_unlock(upperdentry->d_inode);
>> @@ -260,7 +260,7 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
>>                 stat->nlink = dentry->d_inode->i_nlink;
>>
>>  out:
>> -       revert_creds(old_cred);
>> +       ovl_revert_creds(old_cred);
>>
>>         return err;
>>  }
>> @@ -303,7 +303,7 @@ int ovl_permission(struct inode *inode, int mask)
>>
>>         old_cred = ovl_override_creds(inode->i_sb);
>>         err = inode_permission(realinode, mask);
>> -       revert_creds(old_cred);
>> +       ovl_revert_creds(old_cred);
>>
>>         return err;
>>  }
>> @@ -320,7 +320,7 @@ static const char *ovl_get_link(struct dentry *dentry,
>>
>>         old_cred = ovl_override_creds(dentry->d_sb);
>>         p = vfs_get_link(ovl_dentry_real(dentry), done);
>> -       revert_creds(old_cred);
>> +       ovl_revert_creds(old_cred);
>>         return p;
>>  }
>>
>> @@ -363,7 +363,7 @@ int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char *name,
>>                 WARN_ON(flags != XATTR_REPLACE);
>>                 err = vfs_removexattr(realdentry, name);
>>         }
>> -       revert_creds(old_cred);
>> +       ovl_revert_creds(old_cred);
>>
>>         /* copy c/mtime */
>>         ovl_copyattr(d_inode(realdentry), inode);
>> @@ -384,7 +384,7 @@ int ovl_xattr_get(struct dentry *dentry, struct inode *inode, const char *name,
>>
>>         old_cred = ovl_override_creds(dentry->d_sb);
>>         res = vfs_getxattr(realdentry, name, value, size);
>> -       revert_creds(old_cred);
>> +       ovl_revert_creds(old_cred);
>>         return res;
>>  }
>>
>> @@ -408,7 +408,7 @@ ssize_t ovl_listxattr(struct dentry *dentry, char *list, size_t size)
>>
>>         old_cred = ovl_override_creds(dentry->d_sb);
>>         res = vfs_listxattr(realdentry, list, size);
>> -       revert_creds(old_cred);
>> +       ovl_revert_creds(old_cred);
>>         if (res <= 0 || size == 0)
>>                 return res;
>>
>> @@ -443,7 +443,7 @@ struct posix_acl *ovl_get_acl(struct inode *inode, int type)
>>
>>         old_cred = ovl_override_creds(inode->i_sb);
>>         acl = get_acl(realinode, type);
>> -       revert_creds(old_cred);
>> +       ovl_revert_creds(old_cred);
>>
>>         return acl;
>>  }
>> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
>> index aa012b6bd46e..b73e5f7aea2e 100644
>> --- a/fs/overlayfs/namei.c
>> +++ b/fs/overlayfs/namei.c
>> @@ -1074,7 +1074,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>>                         goto out_free_oe;
>>         }
>>
>> -       revert_creds(old_cred);
>> +       ovl_revert_creds(old_cred);
>>         if (origin_path) {
>>                 dput(origin_path->dentry);
>>                 kfree(origin_path);
>> @@ -1101,7 +1101,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>>         kfree(upperredirect);
>>  out:
>>         kfree(d.redirect);
>> -       revert_creds(old_cred);
>> +       ovl_revert_creds(old_cred);
>>         return ERR_PTR(err);
>>  }
>>
>> @@ -1155,7 +1155,7 @@ bool ovl_lower_positive(struct dentry *dentry)
>>                         dput(this);
>>                 }
>>         }
>> -       revert_creds(old_cred);
>> +       ovl_revert_creds(old_cred);
>>
>>         return positive;
>>  }
>> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
>> index 5e45cb3630a0..6f8b6f9ff357 100644
>> --- a/fs/overlayfs/overlayfs.h
>> +++ b/fs/overlayfs/overlayfs.h
>> @@ -208,6 +208,7 @@ int ovl_want_write(struct dentry *dentry);
>>  void ovl_drop_write(struct dentry *dentry);
>>  struct dentry *ovl_workdir(struct dentry *dentry);
>>  const struct cred *ovl_override_creds(struct super_block *sb);
>> +void ovl_revert_creds(const struct cred *oldcred);
>>  struct super_block *ovl_same_sb(struct super_block *sb);
>>  int ovl_can_decode_fh(struct super_block *sb);
>>  struct dentry *ovl_indexdir(struct super_block *sb);
>> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
>> index ec237035333a..e38eea8104be 100644
>> --- a/fs/overlayfs/ovl_entry.h
>> +++ b/fs/overlayfs/ovl_entry.h
>> @@ -20,6 +20,7 @@ struct ovl_config {
>>         bool nfs_export;
>>         int xino;
>>         bool metacopy;
>> +       bool override_creds;
>>  };
>>
>>  struct ovl_sb {
>> diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
>> index cc8303a806b4..ec591b49e902 100644
>> --- a/fs/overlayfs/readdir.c
>> +++ b/fs/overlayfs/readdir.c
>> @@ -289,7 +289,7 @@ static int ovl_check_whiteouts(struct dentry *dir, struct ovl_readdir_data *rdd)
>>                 }
>>                 inode_unlock(dir->d_inode);
>>         }
>> -       revert_creds(old_cred);
>> +       ovl_revert_creds(old_cred);
>>
>>         return err;
>>  }
>> @@ -921,7 +921,7 @@ int ovl_check_empty_dir(struct dentry *dentry, struct list_head *list)
>>
>>         old_cred = ovl_override_creds(dentry->d_sb);
>>         err = ovl_dir_read_merged(dentry, list, &root);
>> -       revert_creds(old_cred);
>> +       ovl_revert_creds(old_cred);
>>         if (err)
>>                 return err;
>>
>> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
>> index 0116735cc321..1669d4fa7ad8 100644
>> --- a/fs/overlayfs/super.c
>> +++ b/fs/overlayfs/super.c
>> @@ -56,6 +56,11 @@ module_param_named(xino_auto, ovl_xino_auto_def, bool, 0644);
>>  MODULE_PARM_DESC(ovl_xino_auto_def,
>>                  "Auto enable xino feature");
>>
>> +static bool __read_mostly ovl_default_override_creds = true;
>
> Please stick to conventions - ovl_override_creds_def.
>
>> +module_param_named(override_creds, ovl_default_override_creds, bool, 0644);
>> +MODULE_PARM_DESC(ovl_default_override_creds,
>> +                "Use mounter's credentials for accesses");
>> +
>>  static void ovl_entry_stack_free(struct ovl_entry *oe)
>>  {
>>         unsigned int i;
>> @@ -362,6 +367,8 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
>>         if (ofs->config.metacopy != ovl_metacopy_def)
>>                 seq_printf(m, ",metacopy=%s",
>>                            ofs->config.metacopy ? "on" : "off");
>> +       seq_show_option(m, "override_creds",
>
> show only if != ovl_override_creds_def
>
>> +                       ofs->config.override_creds ? "on" : "off");
>>         return 0;
>>  }
>>
>> @@ -401,6 +408,8 @@ enum {
>>         OPT_XINO_AUTO,
>>         OPT_METACOPY_ON,
>>         OPT_METACOPY_OFF,
>> +       OPT_OVERRIDE_CREDS_ON,
>> +       OPT_OVERRIDE_CREDS_OFF,
>>         OPT_ERR,
>>  };
>>
>> @@ -419,6 +428,8 @@ static const match_table_t ovl_tokens = {
>>         {OPT_XINO_AUTO,                 "xino=auto"},
>>         {OPT_METACOPY_ON,               "metacopy=on"},
>>         {OPT_METACOPY_OFF,              "metacopy=off"},
>> +       {OPT_OVERRIDE_CREDS_ON,         "override_creds=on"},
>> +       {OPT_OVERRIDE_CREDS_OFF,        "override_creds=off"},
>>         {OPT_ERR,                       NULL}
>>  };
>>
>> @@ -477,6 +488,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>>         config->redirect_mode = kstrdup(ovl_redirect_mode_def(), GFP_KERNEL);
>>         if (!config->redirect_mode)
>>                 return -ENOMEM;
>> +       config->override_creds = ovl_default_override_creds;
>>
>>         while ((p = ovl_next_opt(&opt)) != NULL) {
>>                 int token;
>> @@ -557,6 +569,14 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>>                         config->metacopy = false;
>>                         break;
>>
>> +               case OPT_OVERRIDE_CREDS_ON:
>> +                       config->override_creds = true;
>> +                       break;
>> +
>> +               case OPT_OVERRIDE_CREDS_OFF:
>> +                       config->override_creds = false;
>> +                       break;
>> +
>>                 default:
>>                         pr_err("overlayfs: unrecognized mount option \"%s\" or missing value\n", p);
>>                         return -EINVAL;
>> @@ -1549,7 +1569,8 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>>                        ovl_dentry_lower(root_dentry), NULL);
>>
>>         sb->s_root = root_dentry;
>> -
>> +       if (!ofs->config.override_creds)
>> +               pr_warn("overlayfs: override_creds=off, caller credentials may not be enough to delete file or directories, create nodes, or search directories.\n");
>
> The audience is someone that has this feature on by mistake or someone
> that turn it
> on without understanding what it does. I am not sure that this is
> scary enough, but
> I don't have a better suggestion.
> Will let others state their opinion.

I don't think we need any warning message, writing down the rules in
the documentation should be enough.

Thanks,
Miklos

Re: [PATCH v6 2/2] overlayfs: override_creds=off option bypass creator_cred

[Mark Salyzyn]

On 11/06/2018 12:39 AM, Miklos Szeredi wrote:
> On Mon, Nov 5, 2018 at 7:47 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Mon, Nov 5, 2018 at 8:22 PM Mark Salyzyn <salyzyn@android.com> wrote:
>>> @@ -1549,7 +1569,8 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>>>                         ovl_dentry_lower(root_dentry), NULL);
>>>
>>>          sb->s_root = root_dentry;
>>> -
>>> +       if (!ofs->config.override_creds)
>>> +               pr_warn("overlayfs: override_creds=off, caller credentials may not be enough to delete file or directories, create nodes, or search directories.\n");
>> The audience is someone that has this feature on by mistake or someone
>> that turn it
>> on without understanding what it does. I am not sure that this is
>> scary enough, but
>> I don't have a better suggestion.
>> Will let others state their opinion.
> I don't think we need any warning message, writing down the rules in
> the documentation should be enough.
I would be pleased to remove them, but maybe more historical background 
is required in the documentation (see below [TL;DR])? I have been told 
to not talk rationalization, history, use cases; but only about side 
effects in the documentation.

Yes, the documentation (in the v7 patch set) cites this problem, so can 
remove this pr_warn in a v8 respin. Does anyone disagree? will respin by 
EOD if nothing said.

[TL;DR]

In 4.4 the default behaviour was effectively !override_creds since the 
mounter or creator MAC and DAC credentials wrapping the existing caller 
credentials had not yet been added until later. Except at that time the 
capabilities were temporarily elevated inside the overlayfs driver 
during the cited operations to (blindly) permit these few.

When the mounter's MAC and DAC credentials were added (later), security 
was greatly improved by not counting on the elevated DAC, but it broke 
the expected 4.4 user space API. So in 4.9 and higher Android will 
require this patch to restore the behaviour that supports 
non-overlapping MAC credentials. But we chose _not_ to re-add the 
(inadvisable) hard coded elevated credentials for these specific 
accesses thus requiring the caller to _have_ the DAC credentials to 
perform them.

- For those using the 4.4 way of doing things, these noted operations work.
- For those using the 4.9 way of doing things, the non-overlapping 
creator and caller MAC credentials broke.
- For latest, this patch brought back the support for non-overlapping 
MAC credentials, without the security issues of the 4.4 implementation, 
but alas breaks the 4.4 way of doing things as noted in this warning 
message.

-- Mark

Re: [PATCH v6 0/2]

[Amir Goldstein]

On Mon, Nov 5, 2018 at 8:22 PM Mark Salyzyn <salyzyn@android.com> wrote:
>
> overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh
>
> Assumption never checked, should fail if the mounter creds are not
> sufficient.
>
> overlayfs: override_creds=off option bypass creator_cred
>
> By default, all access to the upper, lower and work directories is the
> recorded mounter's MAC and DAC credentials.  The incoming accesses are
> checked against the caller's credentials.
>
> If the principles of least privilege are applied, the mounter's
> credentials might not overlap the credentials of the caller's when
> accessing the overlayfs filesystem.  For example, a file that a lower
> DAC privileged caller can execute, is MAC denied to the generally
> higher DAC privileged mounter, to prevent an attack vector.
>
> We add the option to turn off override_creds in the mount options; all
> subsequent operations after mount on the filesystem will be only the
> caller's credentials.  The module boolean parameter and mount option
> override_creds is also added as a presence check for this "feature",
> existence of /sys/module/overlay/parameters/overlay_creds
>
> Signed-off-by: Mark Salyzyn <salyzyn@android.com>
> Cc: Miklos Szeredi <miklos@szeredi.hu>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Vivek Goyal <vgoyal@redhat.com>
> Cc: Eric W. Biederman <ebiederm@xmission.com>
> Cc: Amir Goldstein <amir73il@gmail.com>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Stephen Smalley <sds@tycho.nsa.gov>
> Cc: linux-unionfs@vger.kernel.org
> Cc: linux-doc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
>
> v2:
> - Forward port changed attr to stat, resulting in a build error.
> - altered commit message.
>
> v3:
> - Change name from caller_credentials / creator_credentials to the
>   boolean override_creds.
> - Changed from creator to mounter credentials.
> - Updated and fortified the documentation.
> - Added CONFIG_OVERLAY_FS_OVERRIDE_CREDS
>
> v4:
> - spelling and grammar errors in text
>
> v5:
> - beefed up the caveats in the Documentation
> - Is dependent on
>   "overlayfs: check CAP_DAC_READ_SEARCH before issuing exportfs_decode_fh"
>   "overlayfs: check CAP_MKNOD before issuing vfs_whiteout"
> - Added prwarn when override_creds=off
>
> v6:
> - Drop CONFIG_OVERLAY_FS_OVERRIDE_CREDS.
> - Do better with the documentation, drop rationalizations.
> - pr_warn message adjusted to report consequences.

For future reference, the cover letter is meant for introduction,
not for concatenating all commit messages...
Not sure how you generated this text.
I think you were looking for git format-patch --cover.
Anyway, IMO, having the patch revision in the cover letter is sufficient
and you needn't bother with patch revision in every single patch, unless
there is a good reason.

Thanks,
Amir.