Message-ID: <20250521-blusen-bequem-4857e2ce9155@brauner>
Date: Wed, 21 May 2025 13:20:04 +0200
From: Christian Brauner <brauner@...nel.org>
To: Amir Goldstein <amir73il@...il.com>
Cc: André Almeida <andrealmeid@...lia.com>, 
	Miklos Szeredi <miklos@...redi.hu>, linux-unionfs@...r.kernel.org, linux-kernel@...r.kernel.org, 
	kernel-dev@...lia.com, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH] ovl: Allow mount options to be parsed on remount

On Wed, May 21, 2025 at 12:35:57PM +0200, Amir Goldstein wrote:
> On Wed, May 21, 2025 at 8:45 AM André Almeida <andrealmeid@...lia.com> wrote:
> >
> > Allow mount options to be parsed on remount when using the new mount(8)
> > API. This allows to give a precise error code to userspace when the
> > remount is using wrong arguments instead of a generic -EINVAL error.
> >
> > Signed-off-by: André Almeida <andrealmeid@...lia.com>
> > ---
> > Hi folks,
> >
> > I was playing with xfstest with overlayfs and I got those fails:
> >
> > $ sudo TEST_DIR=/tmp/dir1 TEST_DEV=/dev/vdb SCRATCH_DEV=/dev/vdc SCRATCH_MNT=/tmp/dir2 ./check -overlay
> 
> I advise you to set the base FSTYP as README.overlay suggests
> Some tests may require it to run properly or to run at all.
> Probably related to failures you are seeing though...
> 
> > ...
> > Failures: generic/294 generic/306 generic/452 generic/599 generic/623 overlay/035
> > Failed 6 of 859 tests
> >
> > 5 of those 6 fails were related to the same issue, where fsconfig
> > syscall returns EINVAL instead of EROFS:
> >
> 
> I see the test generic/623 failure - this test needs to be fixed for overlay
> or not run on overlayfs.
> 
> I do not see those other 5 failures although before running the test I did:
> export LIBMOUNT_FORCE_MOUNT2=always
> 
> Not sure what I am doing differently.
> 
> > -mount: cannot remount device read-write, is write-protected
> > +mount: /tmp/dir2/ovl-mnt: fsconfig() failed: overlay: No changes allowed in reconfigure
> >
> > I tracked down the origin of this issue being commit ceecc2d87f00 ("ovl:
> > reserve ability to reconfigure mount options with new mount api"), where
> > ovl_parse_param() was modified to reject any reconfiguration when using
> > the new mount API, returning -EINVAL. This was done to avoid non-sense
> > parameters being accepted by the new API, as exemplified in the commit
> > message:
> >
> >         mount -t overlay overlay -o lowerdir=/mnt/a:/mnt/b,upperdir=/mnt/upper,workdir=/mnt/work /mnt/merged
> >
> >     and then issue a remount via:
> >
> >             # force mount(8) to use mount(2)
> >             export LIBMOUNT_FORCE_MOUNT2=always
> >             mount -t overlay overlay -o remount,WOOTWOOT,lowerdir=/DOESNT-EXIST /mnt/merged
> >
> >     with completely nonsensical mount options whatsoever it will succeed
> >     nonetheless.
> >
> > However, after manually reverting such commit, I found out that
> > currently those nonsensical mount options are being reject by the
> > kernel:
> >
> > $ mount -t overlay overlay -o remount,WOOTWOOT,lowerdir=/DOESNT-EXIST /mnt/merged
> > mount: /mnt/merged: fsconfig() failed: overlay: Unknown parameter 'WOOTWOOT'.
> >
> > $ mount -t overlay overlay -o remount,lowerdir=/DOESNT-EXIST /mnt/merged
> > mount: /mnt/merged: special device overlay does not exist.
> >        dmesg(1) may have more information after failed mount system call
> >
> > And now 5 tests are passing because the code can now returns EROFS:
> > Failures: generic/623
> > Failed 1 of 1 tests
> >
> > So this patch basically allows for the parameters to be parsed and to
> > return an appropriated error message instead of a generic EINVAL one.
> >
> > Please let me know if this looks like going in the right direction.
> 
> The purpose of the code that you reverted was not to disallow
> nonsensical arguments.
> The purpose was to not allow using mount arguments that
> overlayfs cannot reconfigure.
> 
> Changing rw->ro should be allowed if no other arguments are
> changed, but I cannot tell you for sure if and how this was implemented.
> Christian?
> 
> >
> > Thanks!
> > ---
> >  fs/overlayfs/params.c | 9 ---------
> >  1 file changed, 9 deletions(-)
> >
> > diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c
> > index f42488c019572479d8fdcfc1efd62b21d2995875..f6b7acc0fee6174c48fcc8b87481fbcb60e6d421 100644
> > --- a/fs/overlayfs/params.c
> > +++ b/fs/overlayfs/params.c
> > @@ -600,15 +600,6 @@ static int ovl_parse_param(struct fs_context *fc, struct fs_parameter *param)
> >                  */
> >                 if (fc->oldapi)
> >                         return 0;
> > -
> > -               /*
> > -                * Give us the freedom to allow changing mount options
> > -                * with the new mount api in the future. So instead of
> > -                * silently ignoring everything we report a proper
> > -                * error. This is only visible for users of the new
> > -                * mount api.
> > -                */
> > -               return invalfc(fc, "No changes allowed in reconfigure");
> >         }
> >
> >         opt = fs_parse(fc, ovl_parameter_spec, param, &result);
> >
> 
> NACK on this as it is.
> 
> Possibly, we need to identify the "only change RDONLY" case
> and allow only that.

Agreed. And this patch is seriously buggy. Just look at this:

> > However, after manually reverting such commit, I found out that
> > currently those nonsensical mount options are being reject by the
> > kernel:
> >
> > $ mount -t overlay overlay -o remount,WOOTWOOT,lowerdir=/DOESNT-EXIST /mnt/merged
> > mount: /mnt/merged: fsconfig() failed: overlay: Unknown parameter 'WOOTWOOT'.
> >
> > $ mount -t overlay overlay -o remount,lowerdir=/DOESNT-EXIST /mnt/merged
> > mount: /mnt/merged: special device overlay does not exist.
> >        dmesg(1) may have more information after failed mount system call

Well of course that fails because the lowedir you're pointing this at
doesn't exist. But consider someone passing a valid lowerdir path or
other valid options then suddenly we're changing the lowerdir parameters
for a running overlayfs instance which is obviously an immediate
security issue because we've just managed to create UAFs all over the
place.

Either the EINVAL for the new mount api is removed and we continue
unconditionally returning 0 for both the new and old mount api or
we allow a limited set of safe remount options.

But changing layers definitely isn't one of them and unconditionally
letting this code reach fs_parse() is an absolute nono.

Powered by blists - more mailing lists