[PATCH 0/3] Misc. redirect_dir=nofollow fixes

[PATCH 0/3] Misc. redirect_dir=nofollow fixes

[Amir Goldstein]

Miklos, Vivek,

Following discussion on following an unsafe non-dir origin [1]
and in a addition to a fix for the reported null uuid case [2] and to
Vivek's doc clarification [3], I am proposing to piggy back existing
config redirect_dir=nofollow to also not follow non-dir origin.

Like in the case of non-dir origin, following redirects behavior was
added with no opt-out option in kernel v4.10.  Later security concerns
about following malformed redirects resulted in the redirect_dir=nofollow
config option.

Without giving too much thought into how unsafe it can be to follow
a bad origin, there is very low motication IMO to follow non-dir origin
with redirect_dir=nofollow, because it is a configuration that prefers
safety over correctness, so it just seems like the right thing to do.

The first two patches are independent bug fixes related to read-only
NFS export, which can be taken regardless of non-dir origin nofollow.
FYI, I found those bugs because I am using ro,index=off NFS export
configuration for the new overlay fsnotify snaphsot series.

Thanks,
Amir.

[1] https://lore.kernel.org/linux-unionfs/CAJfpegv9h7ubuGy_6K4OCdZd3R7Z4HGmCDB2L7mO5bVoGd6MSA@mail.gmail.com/
[2] https://lore.kernel.org/linux-unionfs/20200708131613.30038-1-amir73il@gmail.com/
[3] https://lore.kernel.org/linux-unionfs/20200709140220.GC150543@redhat.com/

Amir Goldstein (3):
  ovl: force read-only sb on failure to create index dir
  ovl: fix mount option checks for nfs_export with no upperdir
  ovl: do not follow non-dir origin with redirect_dir=nofollow

 Documentation/filesystems/overlayfs.rst |  4 +--
 fs/overlayfs/namei.c                    |  2 +-
 fs/overlayfs/super.c                    | 42 ++++++++++++++-----------
 3 files changed, 27 insertions(+), 21 deletions(-)

-- 
2.17.1

[PATCH 1/3] ovl: force read-only sb on failure to create index dir

[Amir Goldstein]

With index feature enabled, on failure to create index dir, overlay
is being mounted read-only.  However, we do not forbid user to remount
overlay read-write.  Fix that by setting ofs->workdir to NULL, which
prevents remount read-write.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/super.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 4b7cb2d98203..41d7fe2b8129 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1374,12 +1374,13 @@ static int ovl_get_indexdir(struct super_block *sb, struct ovl_fs *ofs,
 		goto out;
 	}
 
+	/* index dir will act also as workdir */
+	iput(ofs->workdir_trap);
+	ofs->workdir_trap = NULL;
+	dput(ofs->workdir);
+	ofs->workdir = NULL;
 	ofs->indexdir = ovl_workdir_create(ofs, OVL_INDEXDIR_NAME, true);
 	if (ofs->indexdir) {
-		/* index dir will act also as workdir */
-		iput(ofs->workdir_trap);
-		ofs->workdir_trap = NULL;
-		dput(ofs->workdir);
 		ofs->workdir = dget(ofs->indexdir);
 
 		err = ovl_setup_trap(sb, ofs->indexdir, &ofs->indexdir_trap,
@@ -1884,7 +1885,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	if (!ovl_upper_mnt(ofs))
 		sb->s_flags |= SB_RDONLY;
 
-	if (!(ovl_force_readonly(ofs)) && ofs->config.index) {
+	if (!ovl_force_readonly(ofs) && ofs->config.index) {
 		err = ovl_get_indexdir(sb, ofs, oe, &upperpath);
 		if (err)
 			goto out_free_oe;
-- 
2.17.1

[PATCH 2/3] ovl: fix mount option checks for nfs_export with no upperdir

[Amir Goldstein]

Without upperdir mount option, there is no index dir and the dependency
checks nfs_export => index for mount options parsing are incorrect.

Allow the combination nfs_export=on,index=off with no upperdir and move
the check for dependency redirect_dir=nofollow for non-upper mount case
to mount options parsing.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 Documentation/filesystems/overlayfs.rst |  4 ++--
 fs/overlayfs/super.c                    | 31 ++++++++++++++-----------
 2 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
index 660dbaf0b9b8..fcda5d6ba9ac 100644
--- a/Documentation/filesystems/overlayfs.rst
+++ b/Documentation/filesystems/overlayfs.rst
@@ -560,8 +560,8 @@ When the NFS export feature is enabled, all directory index entries are
 verified on mount time to check that upper file handles are not stale.
 This verification may cause significant overhead in some cases.
 
-Note: the mount options index=off,nfs_export=on are conflicting and will
-result in an error.
+Note: the mount options index=off,nfs_export=on are conflicting for a
+read-write mount and will result in an error.
 
 
 Testsuite
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 41d7fe2b8129..3c8b48a2766b 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -602,12 +602,19 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 		}
 	}
 
-	/* Workdir is useless in non-upper mount */
-	if (!config->upperdir && config->workdir) {
-		pr_info("option \"workdir=%s\" is useless in a non-upper mount, ignore\n",
-			config->workdir);
-		kfree(config->workdir);
-		config->workdir = NULL;
+	/* Workdir/index are useless in non-upper mount */
+	if (!config->upperdir) {
+		if (config->workdir) {
+			pr_info("option \"workdir=%s\" is useless in a non-upper mount, ignore\n",
+				config->workdir);
+			kfree(config->workdir);
+			config->workdir = NULL;
+		}
+		if (config->index && index_opt) {
+			pr_info("option \"index=on\" is useless in a non-upper mount, ignore\n");
+			index_opt = false;
+		}
+		config->index = false;
 	}
 
 	err = ovl_parse_redirect_mode(config, config->redirect_mode);
@@ -644,11 +651,13 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 
 	/* Resolve nfs_export -> index dependency */
 	if (config->nfs_export && !config->index) {
-		if (nfs_export_opt && index_opt) {
+		if (!config->upperdir && config->redirect_follow) {
+			pr_info("NFS export requires \"redirect_dir=nofollow\" on non-upper mount, falling back to nfs_export=off.\n");
+			config->nfs_export = false;
+		} else if (nfs_export_opt && index_opt) {
 			pr_err("conflicting options: nfs_export=on,index=off\n");
 			return -EINVAL;
-		}
-		if (index_opt) {
+		} else if (index_opt) {
 			/*
 			 * There was an explicit index=off that resulted
 			 * in this conflict.
@@ -1616,10 +1625,6 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
 	if (!ofs->config.upperdir && numlower == 1) {
 		pr_err("at least 2 lowerdir are needed while upperdir nonexistent\n");
 		return ERR_PTR(-EINVAL);
-	} else if (!ofs->config.upperdir && ofs->config.nfs_export &&
-		   ofs->config.redirect_follow) {
-		pr_warn("NFS export requires \"redirect_dir=nofollow\" on non-upper mount, falling back to nfs_export=off.\n");
-		ofs->config.nfs_export = false;
 	}
 
 	stack = kcalloc(numlower, sizeof(struct path), GFP_KERNEL);
-- 
2.17.1

[PATCH 3/3] ovl: do not follow non-dir origin with redirect_dir=nofollow

[Amir Goldstein]

Following non-dir origin can result in some bugs when underlying layers
are edited offline.  To be on the safe side, do not follow non-dir
origin when not following redirects.  This will make overlay lookup
with "redirect_dir=nofollow" behave as pre kernel v4.12 lookup, before
the introduction of the origin xattr.

Link: https://lore.kernel.org/linux-unionfs/CAJfpegv9h7ubuGy_6K4OCdZd3R7Z4HGmCDB2L7mO5bVoGd6MSA@mail.gmail.com/
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/namei.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index ae1c1216a038..31ee5a519736 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -861,7 +861,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			err = -EREMOTE;
 			goto out;
 		}
-		if (upperdentry && !d.is_dir) {
+		if (upperdentry && !d.is_dir && ofs->config.redirect_follow) {
 			unsigned int origin_ctr = 0;
 
 			/*
-- 
2.17.1

Re: [PATCH 2/3] ovl: fix mount option checks for nfs_export with no upperdir

[Miklos Szeredi]

On Mon, Jul 13, 2020 at 4:19 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> Without upperdir mount option, there is no index dir and the dependency
> checks nfs_export => index for mount options parsing are incorrect.
>
> Allow the combination nfs_export=on,index=off with no upperdir and move
> the check for dependency redirect_dir=nofollow for non-upper mount case
> to mount options parsing.

Okay, but does this combination make any sense?

Thanks,
Miklos

Re: [PATCH 2/3] ovl: fix mount option checks for nfs_export with no upperdir

[Amir Goldstein]

On Tue, Jul 14, 2020 at 5:52 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Mon, Jul 13, 2020 at 4:19 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > Without upperdir mount option, there is no index dir and the dependency
> > checks nfs_export => index for mount options parsing are incorrect.
> >
> > Allow the combination nfs_export=on,index=off with no upperdir and move
> > the check for dependency redirect_dir=nofollow for non-upper mount case
> > to mount options parsing.
>
> Okay, but does this combination make any sense?

Do you mean configuration of non-upper exported to NFS?
Why not? Anyway, we allowed it and regressed it.

Thanks,
Amir.

Re: [PATCH 2/3] ovl: fix mount option checks for nfs_export with no upperdir

[Miklos Szeredi]

On Tue, Jul 14, 2020 at 4:58 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Jul 14, 2020 at 5:52 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Mon, Jul 13, 2020 at 4:19 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > Without upperdir mount option, there is no index dir and the dependency
> > > checks nfs_export => index for mount options parsing are incorrect.
> > >
> > > Allow the combination nfs_export=on,index=off with no upperdir and move
> > > the check for dependency redirect_dir=nofollow for non-upper mount case
> > > to mount options parsing.
> >
> > Okay, but does this combination make any sense?
>
> Do you mean configuration of non-upper exported to NFS?
> Why not? Anyway, we allowed it and regressed it.

Ah, right.  I was confused and thought we'd want to allow
nfs_eport=on,index=on with no upper dir.  But that's nonsense
obviously.

Thanks,
Miklos

>
> Thanks,
> Amir.

Re: [PATCH 2/3] ovl: fix mount option checks for nfs_export with no upperdir

[Amir Goldstein]

On Tue, Jul 14, 2020 at 6:09 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Tue, Jul 14, 2020 at 4:58 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Tue, Jul 14, 2020 at 5:52 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > >
> > > On Mon, Jul 13, 2020 at 4:19 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > >
> > > > Without upperdir mount option, there is no index dir and the dependency
> > > > checks nfs_export => index for mount options parsing are incorrect.
> > > >
> > > > Allow the combination nfs_export=on,index=off with no upperdir and move
> > > > the check for dependency redirect_dir=nofollow for non-upper mount case
> > > > to mount options parsing.
> > >
> > > Okay, but does this combination make any sense?
> >
> > Do you mean configuration of non-upper exported to NFS?
> > Why not? Anyway, we allowed it and regressed it.
>
> Ah, right.  I was confused and thought we'd want to allow
> nfs_eport=on,index=on with no upper dir.  But that's nonsense
> obviously.
>

Heh no.
FYI, I got to this because working on the configuration:
ro,upperdir=...,nfs_eport=on,index=off

for snapshot, since it's a (forced) read-only mount, even though it
has an upperdir and copy ups, it has no upper rename/unlink, so there is
no need for an index to follow from lower to upper nor to follow redirect
from upper to lower.

Thanks,
Amir.

Re: [PATCH 0/3] Misc. redirect_dir=nofollow fixes

[Vivek Goyal]

On Mon, Jul 13, 2020 at 05:19:42PM +0300, Amir Goldstein wrote:
> Miklos, Vivek,
> 
> Following discussion on following an unsafe non-dir origin [1]
> and in a addition to a fix for the reported null uuid case [2] and to
> Vivek's doc clarification [3], I am proposing to piggy back existing
> config redirect_dir=nofollow to also not follow non-dir origin.
> 
> Like in the case of non-dir origin, following redirects behavior was
> added with no opt-out option in kernel v4.10.  Later security concerns
> about following malformed redirects resulted in the redirect_dir=nofollow
> config option.

So what's the security issue you are seeing with malformed origin? If
it indeed is a security threat, then we should probably introduce
another mount option to disable it (instead of reusing redirect_dir,
because that's so unintuitive, IMHO).

Thanks
Vivek
> 
> Without giving too much thought into how unsafe it can be to follow
> a bad origin, there is very low motication IMO to follow non-dir origin
> with redirect_dir=nofollow, because it is a configuration that prefers
> safety over correctness, so it just seems like the right thing to do.
> 
> The first two patches are independent bug fixes related to read-only
> NFS export, which can be taken regardless of non-dir origin nofollow.
> FYI, I found those bugs because I am using ro,index=off NFS export
> configuration for the new overlay fsnotify snaphsot series.
> 
> Thanks,
> Amir.
> 
> [1] https://lore.kernel.org/linux-unionfs/CAJfpegv9h7ubuGy_6K4OCdZd3R7Z4HGmCDB2L7mO5bVoGd6MSA@mail.gmail.com/
> [2] https://lore.kernel.org/linux-unionfs/20200708131613.30038-1-amir73il@gmail.com/
> [3] https://lore.kernel.org/linux-unionfs/20200709140220.GC150543@redhat.com/
> 
> Amir Goldstein (3):
>   ovl: force read-only sb on failure to create index dir
>   ovl: fix mount option checks for nfs_export with no upperdir
>   ovl: do not follow non-dir origin with redirect_dir=nofollow
> 
>  Documentation/filesystems/overlayfs.rst |  4 +--
>  fs/overlayfs/namei.c                    |  2 +-
>  fs/overlayfs/super.c                    | 42 ++++++++++++++-----------
>  3 files changed, 27 insertions(+), 21 deletions(-)
> 
> -- 
> 2.17.1
> 

Re: [PATCH 1/3] ovl: force read-only sb on failure to create index dir

[Vivek Goyal]

On Mon, Jul 13, 2020 at 05:19:43PM +0300, Amir Goldstein wrote:
> With index feature enabled, on failure to create index dir, overlay
> is being mounted read-only.  However, we do not forbid user to remount
> overlay read-write.  Fix that by setting ofs->workdir to NULL, which
> prevents remount read-write.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

This patch does not apply for me. What branch you have generated it
against. I am using 5.8-rc4.

Vivek

> ---
>  fs/overlayfs/super.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 4b7cb2d98203..41d7fe2b8129 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -1374,12 +1374,13 @@ static int ovl_get_indexdir(struct super_block *sb, struct ovl_fs *ofs,
>  		goto out;
>  	}
>  
> +	/* index dir will act also as workdir */
> +	iput(ofs->workdir_trap);
> +	ofs->workdir_trap = NULL;
> +	dput(ofs->workdir);
> +	ofs->workdir = NULL;
>  	ofs->indexdir = ovl_workdir_create(ofs, OVL_INDEXDIR_NAME, true);
>  	if (ofs->indexdir) {
> -		/* index dir will act also as workdir */
> -		iput(ofs->workdir_trap);
> -		ofs->workdir_trap = NULL;
> -		dput(ofs->workdir);
>  		ofs->workdir = dget(ofs->indexdir);
>  
>  		err = ovl_setup_trap(sb, ofs->indexdir, &ofs->indexdir_trap,
> @@ -1884,7 +1885,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>  	if (!ovl_upper_mnt(ofs))
>  		sb->s_flags |= SB_RDONLY;
>  
> -	if (!(ovl_force_readonly(ofs)) && ofs->config.index) {
> +	if (!ovl_force_readonly(ofs) && ofs->config.index) {
>  		err = ovl_get_indexdir(sb, ofs, oe, &upperpath);
>  		if (err)
>  			goto out_free_oe;
> -- 
> 2.17.1
> 

Re: [PATCH 1/3] ovl: force read-only sb on failure to create index dir

[Amir Goldstein]

On Tue, Jul 14, 2020 at 9:18 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Mon, Jul 13, 2020 at 05:19:43PM +0300, Amir Goldstein wrote:
> > With index feature enabled, on failure to create index dir, overlay
> > is being mounted read-only.  However, we do not forbid user to remount
> > overlay read-write.  Fix that by setting ofs->workdir to NULL, which
> > prevents remount read-write.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> This patch does not apply for me. What branch you have generated it
> against. I am using 5.8-rc4.

It's from my ovl-fixes branch.

Sorry I did not notice that it depends on a previous patch that Miklos
just picked up:

"ovl: fix oops in ovl_indexdir_cleanup() with nfs_export=on"

Thanks,
Amir.

Re: [PATCH 1/3] ovl: force read-only sb on failure to create index dir

[Vivek Goyal]

On Tue, Jul 14, 2020 at 09:32:51PM +0300, Amir Goldstein wrote:
> On Tue, Jul 14, 2020 at 9:18 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Mon, Jul 13, 2020 at 05:19:43PM +0300, Amir Goldstein wrote:
> > > With index feature enabled, on failure to create index dir, overlay
> > > is being mounted read-only.  However, we do not forbid user to remount
> > > overlay read-write.  Fix that by setting ofs->workdir to NULL, which
> > > prevents remount read-write.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >
> > This patch does not apply for me. What branch you have generated it
> > against. I am using 5.8-rc4.
> 
> It's from my ovl-fixes branch.
> 
> Sorry I did not notice that it depends on a previous patch that Miklos
> just picked up:
> 
> "ovl: fix oops in ovl_indexdir_cleanup() with nfs_export=on"

I dont see it here.

https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git/log/?h=overlayfs-next

Is there another tree/branch miklos is maintaining which I should use? Or
you just happen to know that Miklos has committed this internally and
not published yet.

Thanks
Vivek

Re: [PATCH 0/3] Misc. redirect_dir=nofollow fixes

[Amir Goldstein]

On Tue, Jul 14, 2020 at 9:07 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Mon, Jul 13, 2020 at 05:19:42PM +0300, Amir Goldstein wrote:
> > Miklos, Vivek,
> >
> > Following discussion on following an unsafe non-dir origin [1]
> > and in a addition to a fix for the reported null uuid case [2] and to
> > Vivek's doc clarification [3], I am proposing to piggy back existing
> > config redirect_dir=nofollow to also not follow non-dir origin.
> >
> > Like in the case of non-dir origin, following redirects behavior was
> > added with no opt-out option in kernel v4.10.  Later security concerns
> > about following malformed redirects resulted in the redirect_dir=nofollow
> > config option.
>
> So what's the security issue you are seeing with malformed origin? If

TBH I never really understood the thread that led to redirect_dir=nofollow.
I don't think anyone has presented a proper use case that can be discussed,
so I just treat this config option as "paranoia" or "don't give me anything that
very old overlay did not give me".
Therefore I suggested piggybacking on it.
Of course if we do, we will need to document that.
BTW, I have another patch that does not follow mismatched lower dir origin
with redirect_dir=nofollow, i.e.:

 bool ovl_verify_lower(struct super_block *sb)
 {
        struct ovl_fs *ofs = sb->s_fs_info;

-       return ofs->config.nfs_export && ofs->config.index;
+       return ofs->config.nfs_export || !ofs->config.redirect_follow;
 }

That makes even more sense for "paranoia" IMO.

Thanks,
Amir.

Re: [PATCH 1/3] ovl: force read-only sb on failure to create index dir

[Amir Goldstein]

On Tue, Jul 14, 2020 at 9:38 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Tue, Jul 14, 2020 at 09:32:51PM +0300, Amir Goldstein wrote:
> > On Tue, Jul 14, 2020 at 9:18 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > >
> > > On Mon, Jul 13, 2020 at 05:19:43PM +0300, Amir Goldstein wrote:
> > > > With index feature enabled, on failure to create index dir, overlay
> > > > is being mounted read-only.  However, we do not forbid user to remount
> > > > overlay read-write.  Fix that by setting ofs->workdir to NULL, which
> > > > prevents remount read-write.
> > > >
> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > >
> > > This patch does not apply for me. What branch you have generated it
> > > against. I am using 5.8-rc4.
> >
> > It's from my ovl-fixes branch.
> >
> > Sorry I did not notice that it depends on a previous patch that Miklos
> > just picked up:
> >
> > "ovl: fix oops in ovl_indexdir_cleanup() with nfs_export=on"
>
> I dont see it here.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git/log/?h=overlayfs-next
>
> Is there another tree/branch miklos is maintaining which I should use? Or
> you just happen to know that Miklos has committed this internally and
> not published yet.
>

Sorry. He just replied to the email "applied" a few hours ago.
If you need the patch it's on my ovl-fixes branch in github.

Thanks,
Amir.

Re: [PATCH 0/3] Misc. redirect_dir=nofollow fixes

[Vivek Goyal]

On Tue, Jul 14, 2020 at 09:42:53PM +0300, Amir Goldstein wrote:
> On Tue, Jul 14, 2020 at 9:07 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Mon, Jul 13, 2020 at 05:19:42PM +0300, Amir Goldstein wrote:
> > > Miklos, Vivek,
> > >
> > > Following discussion on following an unsafe non-dir origin [1]
> > > and in a addition to a fix for the reported null uuid case [2] and to
> > > Vivek's doc clarification [3], I am proposing to piggy back existing
> > > config redirect_dir=nofollow to also not follow non-dir origin.
> > >
> > > Like in the case of non-dir origin, following redirects behavior was
> > > added with no opt-out option in kernel v4.10.  Later security concerns
> > > about following malformed redirects resulted in the redirect_dir=nofollow
> > > config option.
> >
> > So what's the security issue you are seeing with malformed origin? If
> 
> TBH I never really understood the thread that led to redirect_dir=nofollow.
> I don't think anyone has presented a proper use case that can be discussed,

IIUC, idea was that automated mounting can mount a handcrafted upper on
usb hence allow access to directories on host which are otherwise
inaccessible. 

> so I just treat this config option as "paranoia" or "don't give me anything that
> very old overlay did not give me".
> Therefore I suggested piggybacking on it.

Even if it is paranoia, put more unrelated checks under this option does
not make much sense to me. It will make things just more confusing.

Anyway, redirect_dir=nofollow is a thing of past. Now if you want to
not follow origin, then we first need to have a genuine explanation
of why to do that (and not be driven by just paranoia).

> Of course if we do, we will need to document that.

redirect_dir=nofollow resulting in origin not being followed is plain
unintuitive to me. Why not introduce another option if not following
origin is so important.

Thanks
Vivek

> BTW, I have another patch that does not follow mismatched lower dir origin
> with redirect_dir=nofollow, i.e.:
> 
>  bool ovl_verify_lower(struct super_block *sb)
>  {
>         struct ovl_fs *ofs = sb->s_fs_info;
> 
> -       return ofs->config.nfs_export && ofs->config.index;
> +       return ofs->config.nfs_export || !ofs->config.redirect_follow;
>  }
> 
> That makes even more sense for "paranoia" IMO.
> 
> Thanks,
> Amir.
> 

Re: [PATCH 0/3] Misc. redirect_dir=nofollow fixes

[Amir Goldstein]

> > TBH I never really understood the thread that led to redirect_dir=nofollow.
> > I don't think anyone has presented a proper use case that can be discussed,
>
> IIUC, idea was that automated mounting can mount a handcrafted upper on
> usb hence allow access to directories on host which are otherwise
> inaccessible.
>

That is an *idea* described by hand waving.
That is not a threat I can seriously comment on.
How exactly is that USB auto mounted? where to?
How is that related to overlay?

> > so I just treat this config option as "paranoia" or "don't give me anything that
> > very old overlay did not give me".
> > Therefore I suggested piggybacking on it.
>
> Even if it is paranoia, put more unrelated checks under this option does
> not make much sense to me. It will make things just more confusing.
>
> Anyway, redirect_dir=nofollow is a thing of past. Now if you want to
> not follow origin, then we first need to have a genuine explanation
> of why to do that (and not be driven by just paranoia).
>
> > Of course if we do, we will need to document that.
>
> redirect_dir=nofollow resulting in origin not being followed is plain
> unintuitive to me. Why not introduce another option if not following
> origin is so important.
>

Because cluttering the user with more and more config options for
minor and mostly unimportant behaviors is not ideal either.
See what Kconfig help has to say about the config option:

config OVERLAY_FS_REDIRECT_ALWAYS_FOLLOW
        bool "Overlayfs: follow redirects even if redirects are turned off"
        default y

       Disable this to get a possibly more secure configuration, but that
       might not be backward compatible with previous kernels.

That is a VERY generic description that fits not following origin very
well IMO, and not following unverified dir origin as well for that matter.
Nobody outside overlayfs developers knows what "redirects" means
anyway. To me, following non-dir origin sounds exactly the same
as following non-dir metacopy redirect or dir redirect. It's just the
implementation details that differ.

So my claim is that we *can* piggyback on it because I really
don't believe anybody is using this config out there for anything
other than "to be on the safe side".

But I do not make the calls here and it doesn't look like I managed
to convince you to take my side of the argument :-)

Thanks,
Amir.

Re: [PATCH 1/3] ovl: force read-only sb on failure to create index dir

[Miklos Szeredi]

On Mon, Jul 13, 2020 at 4:19 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> With index feature enabled, on failure to create index dir, overlay
> is being mounted read-only.  However, we do not forbid user to remount
> overlay read-write.  Fix that by setting ofs->workdir to NULL, which
> prevents remount read-write.
>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Applied, thanks.

Miklos

Re: [PATCH 1/3] ovl: force read-only sb on failure to create index dir

[Miklos Szeredi]

On Tue, Jul 14, 2020 at 8:38 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Tue, Jul 14, 2020 at 09:32:51PM +0300, Amir Goldstein wrote:
> > On Tue, Jul 14, 2020 at 9:18 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > >
> > > On Mon, Jul 13, 2020 at 05:19:43PM +0300, Amir Goldstein wrote:
> > > > With index feature enabled, on failure to create index dir, overlay
> > > > is being mounted read-only.  However, we do not forbid user to remount
> > > > overlay read-write.  Fix that by setting ofs->workdir to NULL, which
> > > > prevents remount read-write.
> > > >
> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > >
> > > This patch does not apply for me. What branch you have generated it
> > > against. I am using 5.8-rc4.
> >
> > It's from my ovl-fixes branch.
> >
> > Sorry I did not notice that it depends on a previous patch that Miklos
> > just picked up:
> >
> > "ovl: fix oops in ovl_indexdir_cleanup() with nfs_export=on"
>
> I dont see it here.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git/log/?h=overlayfs-next
>
> Is there another tree/branch miklos is maintaining which I should use? Or
> you just happen to know that Miklos has committed this internally and
> not published yet.

Will push shortly to #overlayfs-next.

Thanks,
Miklos

Re: [PATCH 2/3] ovl: fix mount option checks for nfs_export with no upperdir

[Miklos Szeredi]

On Mon, Jul 13, 2020 at 4:19 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> Without upperdir mount option, there is no index dir and the dependency
> checks nfs_export => index for mount options parsing are incorrect.
>
> Allow the combination nfs_export=on,index=off with no upperdir and move
> the check for dependency redirect_dir=nofollow for non-upper mount case
> to mount options parsing.
>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Applied, thanks.

Miklos

Re: [PATCH 1/3] ovl: force read-only sb on failure to create index dir

[Amir Goldstein]

> > I dont see it here.
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git/log/?h=overlayfs-next
> >
> > Is there another tree/branch miklos is maintaining which I should use? Or
> > you just happen to know that Miklos has committed this internally and
> > not published yet.
>
> Will push shortly to #overlayfs-next.
>

Miklos,

Thanks for the notice.
I think it is good practice to announce when #overlayfs-next is updated and
ask if anyone knows of a patch that fell under the chairs.
I see that other maintainers do that sometimes.

I am missing two of my patches that I wonder if you left out intentionally
or did not get to yet:

1. ovl: fix unneeded call to ovl_change_flags()

One month old posting, so maybe it skipped your radar.
Seems like an obvious fix and needed for stable too.

2. ovl: fix lookup of indexed hardlinks with metacopy

From yesterday, so perhaps you did not get to it yet.
v5.8-rc1 regression.

They are both pushed to my rebased ovl-fixes branch.

Thanks,
Amir.

Re: [PATCH 0/3] Misc. redirect_dir=nofollow fixes

[Vivek Goyal]

On Wed, Jul 15, 2020 at 04:56:45PM +0300, Amir Goldstein wrote:
> > > TBH I never really understood the thread that led to redirect_dir=nofollow.
> > > I don't think anyone has presented a proper use case that can be discussed,
> >
> > IIUC, idea was that automated mounting can mount a handcrafted upper on
> > usb hence allow access to directories on host which are otherwise
> > inaccessible.
> >
> 
> That is an *idea* described by hand waving.
> That is not a threat I can seriously comment on.
> How exactly is that USB auto mounted? where to?
> How is that related to overlay?
> 
> > > so I just treat this config option as "paranoia" or "don't give me anything that
> > > very old overlay did not give me".
> > > Therefore I suggested piggybacking on it.
> >
> > Even if it is paranoia, put more unrelated checks under this option does
> > not make much sense to me. It will make things just more confusing.
> >
> > Anyway, redirect_dir=nofollow is a thing of past. Now if you want to
> > not follow origin, then we first need to have a genuine explanation
> > of why to do that (and not be driven by just paranoia).
> >
> > > Of course if we do, we will need to document that.
> >
> > redirect_dir=nofollow resulting in origin not being followed is plain
> > unintuitive to me. Why not introduce another option if not following
> > origin is so important.
> >
> 
> Because cluttering the user with more and more config options for
> minor and mostly unimportant behaviors is not ideal either.
> See what Kconfig help has to say about the config option:
> 
> config OVERLAY_FS_REDIRECT_ALWAYS_FOLLOW
>         bool "Overlayfs: follow redirects even if redirects are turned off"
>         default y
> 
>        Disable this to get a possibly more secure configuration, but that
>        might not be backward compatible with previous kernels.
> 
> That is a VERY generic description that fits not following origin very
> well IMO, and not following unverified dir origin as well for that matter.
> Nobody outside overlayfs developers knows what "redirects" means
> anyway. To me, following non-dir origin sounds exactly the same
> as following non-dir metacopy redirect or dir redirect. It's just the
> implementation details that differ.
> 
> So my claim is that we *can* piggyback on it because I really
> don't believe anybody is using this config out there for anything
> other than "to be on the safe side".

On one hand you are saying redirect=nofollow is paranoia, most people
don't understand it and don't use it. And on top of that you want
to add more to it. Adding more to something which nobody does not
understand and uses, sounds like more trouble to me.

Anyway, before we go further into this, what's the use case. Why
do you want to provide option to disable following origin for non-dir?

Thanks
Vivek

> 
> But I do not make the calls here and it doesn't look like I managed
> to convince you to take my side of the argument :-)
> 
> Thanks,
> Amir.
> 

Re: [PATCH 0/3] Misc. redirect_dir=nofollow fixes

[Amir Goldstein]

On Thu, Jul 16, 2020 at 4:27 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Wed, Jul 15, 2020 at 04:56:45PM +0300, Amir Goldstein wrote:
> > > > TBH I never really understood the thread that led to redirect_dir=nofollow.
> > > > I don't think anyone has presented a proper use case that can be discussed,
> > >
> > > IIUC, idea was that automated mounting can mount a handcrafted upper on
> > > usb hence allow access to directories on host which are otherwise
> > > inaccessible.
> > >
> >
> > That is an *idea* described by hand waving.
> > That is not a threat I can seriously comment on.
> > How exactly is that USB auto mounted? where to?
> > How is that related to overlay?
> >
> > > > so I just treat this config option as "paranoia" or "don't give me anything that
> > > > very old overlay did not give me".
> > > > Therefore I suggested piggybacking on it.
> > >
> > > Even if it is paranoia, put more unrelated checks under this option does
> > > not make much sense to me. It will make things just more confusing.
> > >
> > > Anyway, redirect_dir=nofollow is a thing of past. Now if you want to
> > > not follow origin, then we first need to have a genuine explanation
> > > of why to do that (and not be driven by just paranoia).
> > >
> > > > Of course if we do, we will need to document that.
> > >
> > > redirect_dir=nofollow resulting in origin not being followed is plain
> > > unintuitive to me. Why not introduce another option if not following
> > > origin is so important.
> > >
> >
> > Because cluttering the user with more and more config options for
> > minor and mostly unimportant behaviors is not ideal either.
> > See what Kconfig help has to say about the config option:
> >
> > config OVERLAY_FS_REDIRECT_ALWAYS_FOLLOW
> >         bool "Overlayfs: follow redirects even if redirects are turned off"
> >         default y
> >
> >        Disable this to get a possibly more secure configuration, but that
> >        might not be backward compatible with previous kernels.
> >
> > That is a VERY generic description that fits not following origin very
> > well IMO, and not following unverified dir origin as well for that matter.
> > Nobody outside overlayfs developers knows what "redirects" means
> > anyway. To me, following non-dir origin sounds exactly the same
> > as following non-dir metacopy redirect or dir redirect. It's just the
> > implementation details that differ.
> >
> > So my claim is that we *can* piggyback on it because I really
> > don't believe anybody is using this config out there for anything
> > other than "to be on the safe side".
>
> On one hand you are saying redirect=nofollow is paranoia, most people
> don't understand it and don't use it. And on top of that you want
> to add more to it. Adding more to something which nobody does not
> understand and uses, sounds like more trouble to me.
>

I am sorry, my POV is exactly the opposite of that.
No need to argue about it though ;-)

> Anyway, before we go further into this, what's the use case. Why
> do you want to provide option to disable following origin for non-dir?
>

I started thinking about this because of the squashfs bug report
(replacing lower layers) for which I had sent a patch to automatically
disable non-dir origin.

Reproducing the same problems with underlying fs with uuid is harder
but not impossible to think of a scenario.
My line of thinking is why should force feed the users with a feature they
didn't ask for if it can break something.
The very least we could do is allow users to opt-out.

But then again, if no one complains, we don't really need to do anything.
I have that feature (no follow origin) anyway for snapshot, but I can enable
it only for the snapshot case, so I don't mind if it can be enabled with
another config option or not - just wanted to put it out there for discussion.

Thanks,
Amir.

Re: [PATCH 0/3] Misc. redirect_dir=nofollow fixes

[Vivek Goyal]

On Thu, Jul 16, 2020 at 04:43:22PM +0300, Amir Goldstein wrote:
> On Thu, Jul 16, 2020 at 4:27 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Wed, Jul 15, 2020 at 04:56:45PM +0300, Amir Goldstein wrote:
> > > > > TBH I never really understood the thread that led to redirect_dir=nofollow.
> > > > > I don't think anyone has presented a proper use case that can be discussed,
> > > >
> > > > IIUC, idea was that automated mounting can mount a handcrafted upper on
> > > > usb hence allow access to directories on host which are otherwise
> > > > inaccessible.
> > > >
> > >
> > > That is an *idea* described by hand waving.
> > > That is not a threat I can seriously comment on.
> > > How exactly is that USB auto mounted? where to?
> > > How is that related to overlay?
> > >
> > > > > so I just treat this config option as "paranoia" or "don't give me anything that
> > > > > very old overlay did not give me".
> > > > > Therefore I suggested piggybacking on it.
> > > >
> > > > Even if it is paranoia, put more unrelated checks under this option does
> > > > not make much sense to me. It will make things just more confusing.
> > > >
> > > > Anyway, redirect_dir=nofollow is a thing of past. Now if you want to
> > > > not follow origin, then we first need to have a genuine explanation
> > > > of why to do that (and not be driven by just paranoia).
> > > >
> > > > > Of course if we do, we will need to document that.
> > > >
> > > > redirect_dir=nofollow resulting in origin not being followed is plain
> > > > unintuitive to me. Why not introduce another option if not following
> > > > origin is so important.
> > > >
> > >
> > > Because cluttering the user with more and more config options for
> > > minor and mostly unimportant behaviors is not ideal either.
> > > See what Kconfig help has to say about the config option:
> > >
> > > config OVERLAY_FS_REDIRECT_ALWAYS_FOLLOW
> > >         bool "Overlayfs: follow redirects even if redirects are turned off"
> > >         default y
> > >
> > >        Disable this to get a possibly more secure configuration, but that
> > >        might not be backward compatible with previous kernels.
> > >
> > > That is a VERY generic description that fits not following origin very
> > > well IMO, and not following unverified dir origin as well for that matter.
> > > Nobody outside overlayfs developers knows what "redirects" means
> > > anyway. To me, following non-dir origin sounds exactly the same
> > > as following non-dir metacopy redirect or dir redirect. It's just the
> > > implementation details that differ.
> > >
> > > So my claim is that we *can* piggyback on it because I really
> > > don't believe anybody is using this config out there for anything
> > > other than "to be on the safe side".
> >
> > On one hand you are saying redirect=nofollow is paranoia, most people
> > don't understand it and don't use it. And on top of that you want
> > to add more to it. Adding more to something which nobody does not
> > understand and uses, sounds like more trouble to me.
> >
> 
> I am sorry, my POV is exactly the opposite of that.
> No need to argue about it though ;-)
> 
> > Anyway, before we go further into this, what's the use case. Why
> > do you want to provide option to disable following origin for non-dir?
> >
> 
> I started thinking about this because of the squashfs bug report
> (replacing lower layers) for which I had sent a patch to automatically
> disable non-dir origin.
> 
> Reproducing the same problems with underlying fs with uuid is harder
> but not impossible to think of a scenario.
> My line of thinking is why should force feed the users with a feature they
> didn't ask for if it can break something.
> The very least we could do is allow users to opt-out.
> 
> But then again, if no one complains, we don't really need to do anything.
> I have that feature (no follow origin) anyway for snapshot, but I can enable
> it only for the snapshot case, so I don't mind if it can be enabled with
> another config option or not - just wanted to put it out there for discussion.

"origin" seems to be more of an internal detail of overlayfs at this
point of time. So I agree that instead of providing another
opetion to disable it now, we can wait if somebody really runs into
issues. And you can disable it for snapshofts internally if you really
have to.

Thanks
Vivek

Re: [PATCH 3/3] ovl: do not follow non-dir origin with redirect_dir=nofollow

[Miklos Szeredi]

On Mon, Jul 13, 2020 at 4:20 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> Following non-dir origin can result in some bugs when underlying layers
> are edited offline.

Sorry, lost track.  What bugs this results in?

Thanks,
Miklos

  To be on the safe side, do not follow non-dir
> origin when not following redirects.  This will make overlay lookup
> with "redirect_dir=nofollow" behave as pre kernel v4.12 lookup, before
> the introduction of the origin xattr.
>
> Link: https://lore.kernel.org/linux-unionfs/CAJfpegv9h7ubuGy_6K4OCdZd3R7Z4HGmCDB2L7mO5bVoGd6MSA@mail.gmail.com/
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/namei.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index ae1c1216a038..31ee5a519736 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -861,7 +861,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                         err = -EREMOTE;
>                         goto out;
>                 }
> -               if (upperdentry && !d.is_dir) {
> +               if (upperdentry && !d.is_dir && ofs->config.redirect_follow) {
>                         unsigned int origin_ctr = 0;
>
>                         /*
> --
> 2.17.1
>

Re: [PATCH 3/3] ovl: do not follow non-dir origin with redirect_dir=nofollow

[Amir Goldstein]

On Fri, Oct 30, 2020 at 2:05 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Mon, Jul 13, 2020 at 4:20 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > Following non-dir origin can result in some bugs when underlying layers
> > are edited offline.
>
> Sorry, lost track.  What bugs this results in?

Frankly, not in a reported bug, but a theoretical one, so feel free to take
it or leave it. I tried to rationalize it in the cover letter [1].

The bug is that you can have an upper inode with wrong origin
lower inode when re-creating lower fs in a way that rewrites the
unique file handle history of the lower fs. This can result in two non-hardlinks
pointing at the same origin and possibly worse.

Commit a888db310195 ("ovl: fix regression with re-formatted lower squashfs")
solved this problem for  re-created lower squashfs by not following
origin in the
case of lower fs with null uuid.

The case of lower fs with non-null uuid you said was less interesting because
re-creating lower would result in a new uuid and therefore origin will not
be followed to the wrong inode.

However, the uuid=off use case [2] tells us that lower fs with uuid can in-fact
be re-created with the same uuid when a prototype image is being cloned
and modified.

The uuid=off feature was proposed because Virtuozzo change the uuid
of lower image after it has been cloned. But other users may not follow
this practice. As a matter of fact xfs has mount option nouuid exactly for
this case (mount a cloned block device as well as the origin).

Long story short, I just thought it would be nice  for users to have a way to
opt-out of any sort of decoding origin when they want "legacy" overlay
functionality in case we have any more latent origin related bugs.

Thanks,
Amir.

[1] https://lore.kernel.org/linux-unionfs/20200713141945.11719-1-amir73il@gmail.com/
[2] https://lore.kernel.org/linux-unionfs/20201013145954.4274-1-ptikhomirov@virtuozzo.com/

Re: [PATCH 3/3] ovl: do not follow non-dir origin with redirect_dir=nofollow

[Miklos Szeredi]

On Fri, Oct 30, 2020 at 2:20 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Fri, Oct 30, 2020 at 2:05 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Mon, Jul 13, 2020 at 4:20 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > Following non-dir origin can result in some bugs when underlying layers
> > > are edited offline.
> >
> > Sorry, lost track.  What bugs this results in?
>
> Frankly, not in a reported bug, but a theoretical one, so feel free to take
> it or leave it. I tried to rationalize it in the cover letter [1].
>
> The bug is that you can have an upper inode with wrong origin
> lower inode when re-creating lower fs in a way that rewrites the
> unique file handle history of the lower fs. This can result in two non-hardlinks
> pointing at the same origin and possibly worse.
>
> Commit a888db310195 ("ovl: fix regression with re-formatted lower squashfs")
> solved this problem for  re-created lower squashfs by not following
> origin in the
> case of lower fs with null uuid.
>
> The case of lower fs with non-null uuid you said was less interesting because
> re-creating lower would result in a new uuid and therefore origin will not
> be followed to the wrong inode.
>
> However, the uuid=off use case [2] tells us that lower fs with uuid can in-fact
> be re-created with the same uuid when a prototype image is being cloned
> and modified.
>
> The uuid=off feature was proposed because Virtuozzo change the uuid
> of lower image after it has been cloned. But other users may not follow
> this practice. As a matter of fact xfs has mount option nouuid exactly for
> this case (mount a cloned block device as well as the origin).
>
> Long story short, I just thought it would be nice  for users to have a way to
> opt-out of any sort of decoding origin when they want "legacy" overlay
> functionality in case we have any more latent origin related bugs.

Okay, let's just postpone this until a real issue turns up.

Thanks,
Miklos